feat(ui5-li-notification, ui5-li-notification-group): introduce new components #1576
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The component covers the main functionaly of the sap.m.NotificationListItem. It is meant to display a notification and has a rich set of properties and slots to do it. Missing parts - no tests - no rtl support - not tested on all browser device matrix
vladitasev
previously requested changes
May 5, 2020
There was a problem hiding this comment.
Looks fantastic!
Tested on Chrome and IE. Works great, everyting accessible by keyborard, some minor IE bugs (Avatar initials not centered, which is not related to your change, and Action button size is a bit off).
Next steps:
- In the sample page, have one more notification list open from the footer of the test page in a popover. This will be the real use case. We can detect future issues with such a sample. I expect this component to be used inside a popover almost always (but it's the user who opens it there, not us).
- Come up with a solution for having "truncate" and "noTruncation" - maybe rename one of them
- The "Show more" feature doesn't seem to work fully yet: If I set showMore to true, and truncate to true, the Show more button appears, but it doesn't do anything yet.
- Before finalizing the feature, maybe it will be good to find out how easy/hard it will be to mix the items with groups, and most of all, if it will be possible to reuse the list internally for the group.
ilhan007
changed the title
ilhan007
changed the title
fifoosid
suggested changes
May 12, 2020
| * @defaultvalue false | ||
| * @public | ||
| */ | ||
| showClose: { |
There was a problem hiding this comment.
I think it would be better if by default the close button is present, so the property is something like hideClose
vladitasev
requested changes
May 12, 2020
There was a problem hiding this comment.
Looks amazing overall!
Nothing major in the code, really, but there are bugs on IE:
- both item/group, when you press "tab", you get a js error and the app crashes:
: Array.prototype.slice: 'this' is null or undefined - keyboard navigation does not work, pressing down up scrolls the page instead.
For this chage, just fix the js error, we can do the keyboard handling separately later. I have exactly the same issue in Tree.js, up/down don't work, probably calling super._onkeydown() fails somehow both here and in the tree.
|
Please also resolve conflicts |
vladitasev
previously approved these changes
May 13, 2020
fifoosid
previously approved these changes
May 13, 2020
vladitasev
approved these changes
May 13, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The ui5-li-notification and ui5-li-notification-group components cover the main functionality of their equivalents in openui5 - sap.m.NotificationListItem and sap.m.NotificationGroup. They are meant to display app notifications and they possess a rich set of properties and slots to achieve it.
Fixes: #1478