Fix reactions style #17869
Fix reactions style #17869
Conversation
…d reaction buttons
toy
force-pushed
the
fix-reactions-style
branch
from
edfe557 to
213216d
Compare
|
@marcalcobe Certainly, thank you! |
app/components/work_packages/activities_tab/journals/item_component/add_reactions.sass
Outdated
Show resolved
Hide resolved
app/components/work_packages/activities_tab/journals/item_component/reactions.sass
Outdated
Show resolved
Hide resolved
| @@ -1,6 +1,9 @@ | |||
| .op-emoji-reactions--gap | |||
| row-gap: var(--base-size-4, 4px) | |||
|
|
|||
| .op-reactions-button .Button-label | |||
There was a problem hiding this comment.
🟢 One thing to watch out for is breaking changes from primer changing class names- I reckon the odds are low but IIRC we try to avoid styling against primer internal class names for this reason.
There was a problem hiding this comment.
I'd strongly vote against this whole block. As @akabiru said before, if Primer changes the class name, this will break without us noticing it. Further, what is even more important to me: You are overwriting the Primer default behaviour here which in itself is something that we should avoid. There are defined styles for the gap between the label and the trailing content. We should stick with that.
@toy I like the font size adjustment but I'm having second thoughts about the rounded emoji counters- one point @psatyal made earlier in the design was avoid drawing attention to emojis reactions - the rounded nature calls them out visually more than I'd like and they can be perceived as notifications as that's what we use for the notification center- add the tab counters and it seems like too much visual noise. 
|
|
Hi @toy, These are very interesting suggestions for improvement. Thank you. Could I ask you what triggered this work? I don't see a work package for this feature, so I don't have context on what motivated it or why we are priotising this. Generally, we don't make direct changes to the design without first writing a feature with acceptance criteria. Having a work packge for work we're doing is important so so we can:
As for the changes themselves, I think the reduced padding is definitely an improvement.
|
There was a problem hiding this comment.
Hi @toy
I appreaciate your proactiveness 👍 I agree with @psatyal, it is hard to evaluate that change without a ticket assigned to it which is specified and accepted. Of course we can all give our two scents to it, but in the end we have a product team whose task it is to think about a consistent and thourough UI. We should not simply skip that.
I added some remarks to the code but would like to wait for the feedback of the product team.
| @@ -1,6 +1,9 @@ | |||
| .op-emoji-reactions--gap | |||
| row-gap: var(--base-size-4, 4px) | |||
|
|
|||
| .op-reactions-button .Button-label | |||
There was a problem hiding this comment.
I'd strongly vote against this whole block. As @akabiru said before, if Primer changes the class name, this will break without us noticing it. Further, what is even more important to me: You are overwriting the Primer default behaviour here which in itself is something that we should avoid. There are defined styles for the gap between the label and the trailing content. We should stick with that.
| }, | ||
| aria: { label: aria_label_text(reaction, data[:users]) }, | ||
| disabled: current_user_cannot_react?, | ||
| classes: %w[op-reactions-button f4 px-2] |
There was a problem hiding this comment.
Please do not use these classes directly. Instead use the system_arguments of Primer. Thus, we can be sure that the code works even if they change the class names in the future
| text: number_of_user_reactions_text(data[:users]), | ||
| test_selector: "reaction-tooltip-#{reaction}" | ||
| ) | ||
| button.with_trailing_visual_counter(count: data[:count]) |
There was a problem hiding this comment.
I like that we are using the default Primer slot here now 👍
|
I created a new work package for this: @toy: I can think you can address my points in the work package (I've copied my comment over) Henriette's ones here since they are more code-related. |
Ticket
Related to OP#40437
What are you trying to accomplish?
Make icons square, a bit increase icon/font size, use visual counter for number of reactions and reduce distance between icons.
Screenshots
Was:
Before decreasing distance:
Before correcting sizes:
Now:
Merge checklist