Refactoring: Notification and Status #941

Open
opened 2020-09-01 14:14:33 +00:00 by hj · 4 comments
Owner

Here's a proposition to refactor some parts of our code to improve clarity, remove the duplication and reduce overall complexity. This would also allow easier creation of proper Mentions/Interactions column.

Problem: Notification either renders a Status or tries to mimic how status looks for non-mentions and uses StatusContent. This is a bit confusing and the "mimics the Status" feels wrong, not to mention causes problems with CSS.

Proposed solution: unify both into an Activity component, which is basically what both Notification and Status have in common - avatar, name/info, timestamp. It would render StatusContent for activities that warrant it (status, mention, favorite, repeat, emoji) or other content for non-status-based activities (follow, move). It would also render the action bar (reply, rt, fav, emoji, dots) if activity is interactive (mention, status).

This would basically be a component that could render both MastoAPI's Notification and Status.

What this proposal does not solve/pitfalls: probably doesn't really makes it easier to make statuses expandable in Notification column/Interactions Timeline since that's a Conversation component and expansion feels like part of Timeline's responsibility, not statuses'

Here's a proposition to refactor some parts of our code to improve clarity, remove the duplication and reduce overall complexity. This would also allow easier creation of proper Mentions/Interactions column. **Problem**: Notification either renders a Status or tries to mimic how status looks for non-mentions and uses StatusContent. This is a bit confusing and the "mimics the Status" feels wrong, not to mention causes problems with CSS. **Proposed solution**: unify both into an Activity component, which is basically what both Notification and Status have in common - avatar, name/info, timestamp. It would render StatusContent for activities that warrant it (status, mention, favorite, repeat, emoji) or other content for non-status-based activities (follow, move). It would also render the action bar (reply, rt, fav, emoji, dots) if activity is interactive (mention, status). This would basically be a component that could render both MastoAPI's Notification and Status. **What this proposal does not solve/pitfalls**: probably doesn't really makes it easier to make statuses expandable in Notification column/Interactions Timeline since that's a Conversation component and expansion feels like part of Timeline's responsibility, not statuses'
Owner

I think it's a wrong abstraction to merge them into one component when that's not how it works logically. Notifications contain statuses, it always goes that way. in my opinion the right abstraction is a notification component wrapping a status component whenever it contains a status which is already close to what we're doing. on top of that if we need to have a unified component you'd just have a NotificationOrStatus component which includes the wrapper or not depending on the data.

I think it's a wrong abstraction to merge them into one component when that's not how it works logically. Notifications contain statuses, it always goes that way. in my opinion the right abstraction is a notification component wrapping a status component whenever it contains a status which is already close to what we're doing. on top of that if we need to have a unified component you'd just have a NotificationOrStatus component which includes the wrapper or not depending on the data.
Author
Owner

It's sorta is NotificationOrStatus but with less awful name. The main idea stems from having to render nearly same things in two different places while trying to keep the consistency.

image

This was somewhat of a pain point when doing last CSS refactoring.

Alternatively there could be ActivityLayout component, ActivityHeader component and such but i feel like doing ActivityLayout would involve doing slots and that would increase architectural complexity, not reduce it.

It's sorta is NotificationOrStatus but with less awful name. The main idea stems from having to render nearly same things in two different places while trying to keep the consistency. ![image](/attachments/f0df958d-e607-4b3b-9060-23177daa79e3) This was somewhat of a pain point when doing last CSS refactoring. Alternatively there could be ActivityLayout component, ActivityHeader component and such but i feel like doing ActivityLayout would involve doing slots and that would increase architectural complexity, not reduce it.
Owner

I wouldn't pick a vague name that overlaps with non-FE terminology like "Activity". if there's just two options an 'Or' name is the most straight forward option imo. more made up terms is not better.

I still have a bit of a feeling that it might be the wrong way to approach the problem in the pic, I agree it needs to be addressed n some way. in my view merging components should be done when it makes sense logically in terms of data and not just in terms of presentation. perhaps some higher level CSS classes could be used, as in just classes that are not owned by status nor notification, but both of them use it to deduplicate code.

I wouldn't pick a vague name that overlaps with non-FE terminology like "Activity". if there's just two options an 'Or' name is the most straight forward option imo. more made up terms is not better. I still have a bit of a feeling that it might be the wrong way to approach the problem in the pic, I agree it needs to be addressed n some way. in my view merging components should be done when it makes sense logically in terms of data and not just in terms of presentation. perhaps some higher level CSS classes could be used, as in just classes that are not owned by status nor notification, but both of them use it to deduplicate code.
Author
Owner

well as far as I understand, all notifications and statuses are essentially activities represented through prism of MastoAPI but maybe I'm wrong.

Having CSS classes that are not part of any component feels really weird and somewhat confusing, and you may end up having to sync three places across each other for some change in one place since CSS still sorta co-depends on HTML structure being right. It would either way end up being a pseudo-component like NotificationOrStatus which only has css in it.

well as far as I understand, all notifications and statuses are essentially activities represented through prism of MastoAPI but maybe I'm wrong. Having CSS classes that are not part of any component feels really weird and somewhat confusing, and you may end up having to sync three places across each other for some change in one place since CSS still sorta co-depends on HTML structure being right. It would either way end up being a pseudo-component like NotificationOrStatus which only has css in it.
Sign in to join this conversation.
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
pleroma/pleroma-fe#941
No description provided.