Skip to content
This repository has been archived by the owner on Nov 6, 2022. It is now read-only.

Feature/colour #214

Merged
merged 8 commits into from
Jan 10, 2019
Merged

Feature/colour #214

merged 8 commits into from
Jan 10, 2019

Conversation

lukecarbis
Copy link
Member

An alternate approach to a colour picker control.

Closes #201.
Resolves #32.

@lukecarbis lukecarbis requested a review from RobStino January 4, 2019 06:05
@lukecarbis
Copy link
Member Author

Here's a template you can use to test the colour picker (just change the field name on line 1 to match your field): https://gist.github.com/lukecarbis/28d70ece69ee97adfbf769011301a573

@RobStino
Copy link
Collaborator

RobStino commented Jan 7, 2019

I'm still QA testing, but noticed the following. Mostly discussed in our call earlier.

  • The text input for the text field should not read only. I'm guessing that any pasted/typed input will mean the control will have to recognise what's being input.
  • The hashed background on the colour indicator for Alpha colour types
  • Hover state you mentioned isn't working
  • The Colour indicator circle size isn't the same height as the text input. Looks funky
  • Need to style the colour indicator for the inspector

@lukecarbis
Copy link
Member Author

Okay – I've updated the PR with those fixes. A couple of notes:

  • The colour indicator circle is now 28px with 1px margin on top and bottom (total 30px), whereas the text input is 30px high. This colour indicator height matches the other colour circles in Gutenberg.
  • Know unfixable (for now) issue: changing the colour in the colour picker, then in the text input, the colour circle doesn't update after the text changes
  • This also fixes an issue where error'ed fields (such as bad URLs or Emails) in the sidebar weren't receiving the correct red border styles

@RobStino
Copy link
Collaborator

RobStino commented Jan 8, 2019

Tested and confirming:

  • Checked background looks great!
  • No more read only. Nice!
  • Sidebar styling looks good

Re the colour indicator circle placement in the editor, is it possible to have it vertically aligned with the center of the text field?

I'm still seeing this No's in the editor and front end as discussed last night. I forgot to add it as a comment here in the PR, so adding it here now.

@lukecarbis
Copy link
Member Author

@RobStino It should be vertically aligned already – screenshot it for me?

Is the "No's" issue a problem with this PR or is it on develop?

@RobStino
Copy link
Collaborator

RobStino commented Jan 8, 2019

Hey @lukecarbis
It's vertically aligned to the top currently.
image

Checked and the "No's" issue is only on this PR branch.

@RobStino
Copy link
Collaborator

RobStino commented Jan 10, 2019

@luke I adjusted that margin further so it's vertically centered.
image
Check it over and merge when you're ready. I've reviewed/approved.

Copy link
Collaborator

@RobStino RobStino 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 to me

@lukecarbis lukecarbis merged commit 5715f33 into develop Jan 10, 2019
@lukecarbis lukecarbis deleted the feature/colour branch January 10, 2019 04:00
lukecarbis added a commit to lukecarbis/block-lab that referenced this pull request Aug 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants