Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Align comments indicator and add hover state to comments avatar #10901

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

banobepascal
Copy link

Change-Id: I4746de9d38ce016e0fdef2d0ef15d810474276b6

Summary

Fixes the alignment, size, and interactivity issues with the number of replies indicator in the comments section, resized to fit seamlessly within the circle, and includes a hover state

Screenshot

Screencast.from.10-01-25.15.38.57.webm

@banobepascal
Copy link
Author

@pedropintosilva will be waiting for your feedback

@banobepascal banobepascal requested a review from eszkadev January 10, 2025 13:06
Copy link
Contributor

@pedropintosilva pedropintosilva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @banobepascal , the sizes look much better. However - you can see this by yourself when testing with an integrator or simply checking staging perf - the border color is overwritten later in

$(authorAvatarImg).css('border-color', color);
so, each comment avatar/bubble gets the same border color as its author. Thus, changing the border color on hover is not the best for UX, maybe better would be to:

  • Hover: Add a subtle box-shadow + opacity to 1
  • Normal state: Set element with opacity 0.9

This way we would have avatars/bubbles more discreet as default with a hover state that would be visible.

Note: It would be good if these changes are only apply for when there is no comment cards ( only bubbles floating due to space needs)

@banobepascal banobepascal force-pushed the private/banobepascal/comments-replie-indicator branch from acb28a8 to e58c2c8 Compare January 16, 2025 21:03
  Added collapsed-comment class to comments when they are collapsed,
  for targeting hover state on avatar imgs

Signed-off-by: Banobe Pascal <[email protected]>
Change-Id: Ice61d1d7893035809976d3c2983a7c3e0133b285
@banobepascal banobepascal force-pushed the private/banobepascal/comments-replie-indicator branch from e58c2c8 to 35634c1 Compare January 16, 2025 21:07
@banobepascal
Copy link
Author

so, each comment avatar/bubble gets the same border color as its author. Thus, changing the border color on hover is not the best for UX, maybe better would be to:

  • Hover: Add a subtle box-shadow + opacity to 1
  • Normal state: Set element with opacity 0.9

This way we would have avatars/bubbles more discreet as default with a hover state that would be visible.

Note: It would be good if these changes are only apply for when there is no comment cards ( only bubbles floating due to space needs)

New improvements
Screencast from 17-01-25 00:20:07.webm

@banobepascal
Copy link
Author

@pedropintosilva please review

@pedropintosilva pedropintosilva requested review from gokaysatir and pedropintosilva and removed request for eszkadev January 17, 2025 14:29
Copy link
Contributor

@pedropintosilva pedropintosilva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good, I would like to have and extra +1 on the commentListSection from Gokay

@pedropintosilva
Copy link
Contributor

the CI failure is unrelated so I will press the retry button and this change is quite safe anyhow

Copy link
Contributor

@gokaysatir gokaysatir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks beautiful. Can we merge this after below one?

There may be merge conflicts with my PR and i would like to have conflicts here instead of in 300 lines of change.

I hope that is not selfish :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: To Test
Development

Successfully merging this pull request may close these issues.

Misaligned and Incorrectly Styled Replies Indicator in Comments Section
3 participants