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

fix(ui5-li-notification-group): align priority icon #2584

Merged
merged 3 commits into from
Jan 12, 2021

Conversation

ilhan007
Copy link
Member

@ilhan007 ilhan007 commented Dec 16, 2020

Background
The avatar slot give users the freedom to add their own images, icons or to use the available ui5-avatar web component. We don't want to restrict the size of the avatar internally, so whenever the users set ui5-avatar size="XS", ui5-avatar size="M", ui5-avatar size="XL".. they get what they expect.
However, we explain that for best experience we recommend using images/avatars 32px X 32px. With this size, the priority icons are perfectly aligned across the NotificationListGroupItem and its children - NotificationLisItem(s).
In all our test pages and playground samples we use the ui5-avatar with size="XS", which results in 32px X 32px, but it turns out a degradation has been introduced and the XS size of the ui5-avatar appears bigger than expected. For that, there is a PR #2582 already merged.
Now the alignment with a custom image 32px X 32px or ui5-avatar with size="XS" will look like on the image below:

Prerequisite: #2582 (merged already)

Change: adjust the margin of the NotificationListGroupItem's expand/collapse button to align the priority icon
and document the recommended avatar size.

Screenshot 2020-12-16 at 12 02 29 PM

@ilhan007 ilhan007 requested a review from a team December 16, 2020 10:16
@@ -28,7 +28,7 @@
}

.ui5-nli-group-toggle-btn {
margin-right: 1rem;
margin-right: 0.75rem;
Copy link
Member

Choose a reason for hiding this comment

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

margin-right: 1.25rem;
margin-left: 0.5rem;

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the feedback, Teo.

Just to clarify, in openui5 the toggle button has margin-right: 0.75rem

.sapMNLGroupCollapseButton {
       margin-right: 0.75rem;
       min-width: 2.25rem;
       max-width: 2.25rem;
}

When I apply margin-right: 0.75 rem the result is: (seems aligned to me)
Screenshot 2020-12-18 at 5 16 54 PM

When I apply margin-right: 1.25rem; and margin-left: 0.5rem; the result is (not quite aligned):
Screenshot 2020-12-18 at 5 24 52 PM

Copy link
Member

@TeodorTaushanov TeodorTaushanov left a comment

Choose a reason for hiding this comment

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

I rechecked it. The alignment is correct.

@ilhan007 ilhan007 merged commit ff247f0 into master Jan 12, 2021
@ilhan007 ilhan007 deleted the fix-notification-prio-alignment branch January 12, 2021 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants