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

551: Fix IdsColorPicker and IdsColor remaining features #746

Merged
merged 14 commits into from
Jun 9, 2022

Conversation

clayinfor
Copy link
Collaborator

@clayinfor clayinfor commented May 31, 2022

Explain the details for making this change. What existing problem does the pull request solve?

Fixes remaining Bugs/Features for a 100% IdsColorPicker Component

Related github/jira issue (required):

Closes #551

Steps necessary to review your pull request (required):

See examples components below:

Included in this Pull Request:

  • An e2e or functional test for the bug or feature.
  • A note to the change log.

@clayinfor clayinfor requested a review from a team as a code owner May 31, 2022 00:42
@clayinfor clayinfor force-pushed the 551-ids-color-picker-remaining-features branch from aaf7203 to cb834da Compare May 31, 2022 00:55
@clayinfor clayinfor force-pushed the 551-ids-color-picker-remaining-features branch from cb834da to 4093b0d Compare May 31, 2022 01:00
Copy link
Contributor

@EdwardCoyle EdwardCoyle left a comment

Choose a reason for hiding this comment

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

Looks pretty good so far! I saw one usability thing with the tooltip -- appears that there's one coming from the field swatch no matter the setting:

Screen Shot 2022-05-31 at 1 13 13 PM

Copy link
Contributor

@andriivarhanov andriivarhanov 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, nothing major, some findings:

  • please fix tests and prod build
  • when setting input value and open popup the value should be focused

image

test/ids-color/ids-color-func-test.ts Show resolved Hide resolved
src/components/ids-color-picker/ids-color-picker.ts Outdated Show resolved Hide resolved
src/components/ids-color-picker/ids-color-picker.ts Outdated Show resolved Hide resolved
src/components/ids-color-picker/ids-color-picker.ts Outdated Show resolved Hide resolved
src/components/ids-color-picker/ids-color-picker.ts Outdated Show resolved Hide resolved
Copy link
Member

@tmcconechy tmcconechy left a comment

Choose a reason for hiding this comment

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

Some smaller issues (mostly minor but worth a quick look if possible)

  1. on http://localhost:4300/ids-color/example.html if you tab through, some have a nice focus state and some are missing. Also can the focus ring have a border radius?
  2. similar on the popup i see a double focus state (outline showing)
    Not show here i couldnt screen shot it:
    Screen Shot 2022-06-01 at 10 16 26 AM
  3. can make the width bigger on the tooltip
    Screen Shot 2022-06-01 at 10 19 55 AM

src/components/ids-color-picker/README.md Outdated Show resolved Hide resolved
src/components/ids-color-picker/demos/index.yaml Outdated Show resolved Hide resolved
src/components/ids-color-picker/ids-color-picker.scss Outdated Show resolved Hide resolved
@tmcconechy
Copy link
Member

@danielortiz1982 danielortiz1982 merged commit 743c4b8 into main Jun 9, 2022
@tmcconechy tmcconechy deleted the 551-ids-color-picker-remaining-features branch August 10, 2022 20:39
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.

IdsColorPicker: Remaining Features/Issues
5 participants