-
Notifications
You must be signed in to change notification settings - Fork 273
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
feat(ui5-color-palette): initial implementation #2731
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, overall much simplified :)
It's getting close to being ready
@@ -0,0 +1,9 @@ | |||
import DataType from "./DataType.js"; | |||
|
|||
class CSSColor extends DataType { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fifoosid Consider using this for the color picker?
packages/main/bundle.esm.js
Outdated
@@ -47,6 +47,8 @@ import Button from "./dist/Button.js"; | |||
import Card from "./dist/Card.js"; | |||
import Carousel from "./dist/Carousel.js"; | |||
import CheckBox from "./dist/CheckBox.js"; | |||
import ColorPalette from "./dist/ColorPalette.js"; | |||
import ColorPaletteEntry from "./dist/ColorPaletteEntry.js"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should import ColorPaletteEntry always (with the ColorPalette).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! I like how this is shaping up. Logic is nicely separated and files are small and easy to read and understand. I'd like you to clear up 2 things:
- IMO better names would be:
ColorPaletteEntry
=>PaletteColor
andui5-color-palette-entry
=>ui5-palette-color
. The logic is that we have a Color Palette, composed of Colors. We can call them Palette Colors because that's what they are - colors of the Color Palette. Let's discuss this via email. I'll follow up on it. - Please look into the
index
logic. Currently, as far as I can see, index is only used for displaying ACC texts and labels. Yet it is bound set inside both.hbs
files and additionally inonBeforeRendering
. IMO just set theindex
property for the colors insideonBeforeRendering
and not in any of the templates. Then just read it in the color class to build the acc stuff. This should be enough.
I think this change is nearly ready overall. We could merge it today :)
…ponents into color-palette-poc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sufficient ARIA support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just tested on IE11 and it does not work at all. Clicking colors has no effect whatsoever. Please check if the problem stems from the fact that events have been bound to the slot. There are minor visual degradations, but the 2 main bugs are:
- not all colors are displayed
- clicking a color does not fire the event
FIXES: #2615