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

feat(ui5-color-palette): initial implementation #2731

Merged
merged 16 commits into from
Feb 11, 2021

Conversation

tsanislavgatev
Copy link
Contributor

@tsanislavgatev tsanislavgatev commented Jan 26, 2021

FIXES: #2615

packages/main/src/ColorPalette.js Show resolved Hide resolved
packages/main/src/ColorPalette.js Show resolved Hide resolved
packages/main/src/ColorPalette.js Show resolved Hide resolved
packages/main/src/ColorPalette.js Outdated Show resolved Hide resolved
packages/main/src/ColorPalette.js Outdated Show resolved Hide resolved
packages/main/src/ColorPalette.js Outdated Show resolved Hide resolved
packages/main/src/ColorPaletteEntry.js Outdated Show resolved Hide resolved
packages/main/src/ColorPalette.js Show resolved Hide resolved
packages/main/src/ColorPaletteEntry.js Outdated Show resolved Hide resolved
packages/main/src/ColorPalette.js Show resolved Hide resolved
Copy link
Contributor

@vladitasev vladitasev left a 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 {
Copy link
Contributor

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?

@@ -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";
Copy link
Member

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).

Copy link
Contributor

@vladitasev vladitasev left a 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 and ui5-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 in onBeforeRendering. IMO just set the index property for the colors inside onBeforeRendering 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 :)

packages/main/src/ColorPalette.js Outdated Show resolved Hide resolved
packages/main/src/ColorPalette.js Outdated Show resolved Hide resolved
packages/main/src/ColorPalette.js Outdated Show resolved Hide resolved
packages/main/src/ColorPalette.js Outdated Show resolved Hide resolved
packages/main/src/ColorPaletteEntry.hbs Outdated Show resolved Hide resolved
packages/main/bundle.esm.js Show resolved Hide resolved
packages/main/src/ColorPalette.js Outdated Show resolved Hide resolved
packages/main/src/ColorPalette.js Outdated Show resolved Hide resolved
ghristanov
ghristanov previously approved these changes Feb 9, 2021
Copy link

@ghristanov ghristanov left a comment

Choose a reason for hiding this comment

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

Sufficient ARIA support.

packages/main/src/ColorPaletteItem.js Outdated Show resolved Hide resolved
packages/main/src/ColorPaletteItem.js Outdated Show resolved Hide resolved
packages/main/src/ColorPaletteItem.js Outdated Show resolved Hide resolved
packages/main/src/i18n/messagebundle.properties Outdated Show resolved Hide resolved
ghristanov
ghristanov previously approved these changes Feb 10, 2021
Copy link
Contributor

@vladitasev vladitasev left a 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

@tsanislavgatev tsanislavgatev merged commit 772424e into SAP:master Feb 11, 2021
@tsanislavgatev tsanislavgatev deleted the color-palette-poc branch February 11, 2021 06:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ColorPalette (ui5-color-palette): New component
5 participants