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

Adds prepend and append to the EuiColorPicker #2819

Merged
merged 8 commits into from
Feb 4, 2020

Conversation

miukimiu
Copy link
Contributor

@miukimiu miukimiu commented Feb 3, 2020

Summary

Part of the #2794

This PR adds a prepend and append to the EuiColorPicker.

color-picker

More toggling options were added into the Forms Color Selection section of the docs.

prepend-append-color-picker

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in IE11 and Firefox
  • Props have proper autodocs
  • Added documentation examples
  • Added or updated jest tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Thanks for adding this. The only visual issue I see is that the input still has rounded corners as seen in the focus and invalid states.

Screen Shot 2020-02-03 at 15 51 31 PM

src/components/color_picker/color_picker.tsx Show resolved Hide resolved
src/components/color_picker/color_picker.tsx Outdated Show resolved Hide resolved
@miukimiu
Copy link
Contributor Author

miukimiu commented Feb 4, 2020

Thanks, @cchaos. I just fixed the border-radius issue:

color-picker-border-radius

@miukimiu miukimiu requested a review from cchaos February 4, 2020 13:24
Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the test

.euiColorPicker__popoverAnchor { ... is still in here. Is this going to change at all?

@@ -360,6 +381,7 @@ export const EuiColorPicker: FunctionComponent<EuiColorPickerProps> = ({
isOpen={isColorSelectorShown}
closePopover={handleFinalSelection}
zIndex={popoverZIndex}
className={popoverClass}
Copy link
Contributor Author

@miukimiu miukimiu Feb 4, 2020

Choose a reason for hiding this comment

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

I just added the missing className euiColorPicker__popoverAnchor into the EuiPopover and the issue with the arrow overlaping is fixed:
Screenshot 2020-02-04 at 16 04 54

This way I don't need to change the .euiColorPicker__popoverAnchor { ... css.

@cchaos
Copy link
Contributor

cchaos commented Feb 4, 2020

Hmmm, sorry I didn't catch this before. But what do you think about where the dropdown happens? Where the left side of the popover is aligned with the prepend versus the actual input.
Screen Shot 2020-02-04 at 11 33 55 AM

@thompsongl
Copy link
Contributor

Hmmm, sorry I didn't catch this before. But what do you think about where the dropdown happens? Where the left side of the popover is aligned with the prepend versus the actual input.
Screen Shot 2020-02-04 at 11 33 55 AM

I missed this, too. But, if we want the alignment to change, we'll need to modify how the popover service works. It currently doesn't accept enough params to account for positioning like that. (Certainly possible; it just may be need to be done in a separate issue)

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

(Certainly possible; it just may be need to be done in a separate issue)

Ok, I'll create an issue. This PR 👍 LGTM!

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.

3 participants