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

12647 fix css color picker #12747

Merged
merged 3 commits into from
Feb 4, 2019
Merged

12647 fix css color picker #12747

merged 3 commits into from
Feb 4, 2019

Conversation

kadencewp
Copy link
Contributor

Description

Tweaked CSS padding for input fields within the colorPicker

Fixes: #12647

How has this been tested?

Tested Manually

Screenshots

Before CSS Change:
color_before_css

After CSS Change:
color_after_css

Types of changes

Added specific CSS styling to the input fields

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@gziolo gziolo added Needs Design Feedback Needs general design feedback. [Type] Bug An existing feature does not function as intended [Package] Components /packages/components labels Dec 14, 2018
Copy link
Contributor

@jasmussen jasmussen left a comment

Choose a reason for hiding this comment

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

I could not reproduce this issue in any browser or on Windows, for the stock Paragraph color picker:

screenshot 2018-12-17 at 09 13 19

However, the issue seems legit, and the code change (per above comments addressed) seems fine. I can imagine this issue appears primarily when using the picker in other contexts, so good to have. Thanks!

@@ -199,6 +199,9 @@
fieldset {
flex: 1;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a linebreak after this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added this to the latest

@@ -35,7 +35,7 @@
position: relative;
}
.components-color-picker__body {
padding: 16px 16px 12px;
padding: 16px 0 12px;
Copy link
Contributor

Choose a reason for hiding this comment

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

While we're here, can you use grid units? Essentially the following instead:

padding: $grid-size-large 0 #{ $grid-size-small * 3 };

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added this to the latest pull

@kadencewp
Copy link
Contributor Author

Just for clarification, the reason you are not seeing this with the paragraph block is that it doesn't use the ColorPicker component, it's using PanelColorSettings.

@gziolo
Copy link
Member

gziolo commented Jan 25, 2019

@kadencethemes can you rebase this PR so we could land this changes in the next plugin release?

@gziolo gziolo added this to the 5.0 (Gutenberg) milestone Jan 25, 2019
@kadencewp
Copy link
Contributor Author

Hey @gziolo I'm not very familiar with what "rebase this PR" means. Could you tell me what I need to do?

@gziolo
Copy link
Member

gziolo commented Jan 28, 2019

@kadencethemes some more details about rebasing a Pull Request you can find in https://github.com/edx/edx-platform/wiki/How-to-Rebase-a-Pull-Request.

When many different people are working on a project simultaneously, pull requests can go stale quickly. A "stale" pull request is one that is no longer up to date with the main line of development, and it needs to be updated before it can be merged into the project.

When working by yourself, it is best to use git pull --rebase master, but if you're pushing to a shared repo, it is best to not do any merging or rebasing until the feature is ready for final testing, and then do a rebase at the very end. This is one reason why it is important to open pull requests whenever you have working code.

If you have a Pull Request branch that cannot be merged into master due to a conflict (this can happen for long-running Pull Request discussions), it's still best to rebase the branch (rather than merge) and resolve any conflicts on your local copy.

Once you have resolved any conflicts locally you can update the Pull Request with git push --force-with-lease.

@gziolo
Copy link
Member

gziolo commented Jan 28, 2019

I also opened #13534 to add a similar note to docs so it was easier to reference in the future if someone wants to learn about rebasing PRs.

@kadencewp
Copy link
Contributor Author

I'm not sure I did that all correct so let me know.

@kadencewp
Copy link
Contributor Author

ok well, I see I did do this wrong, I somehow have 787 file changes. At this point, I messed around for a while trying to undo the merge I apparently did. I can't seem to get that to happen. My experience with all of this is limited, I haven't worked with a large shared project and I really only use Sourcetree on a regular basis for simple version control of my own projects. Would you suggest I simply recreate a pull with this very simple and basic CSS change to the color picker component?

You mentioned: "This is one reason why it is important to open pull requests whenever you have working code." and when this was pulled in it was working and was tested. How do I prevent this in the future if I want to help?

@jasmussen
Copy link
Contributor

@gziolo my hero — I can offer you an emoji medal if you can help out in this PR with a good rebase to fix this up. Do you have time?

@gziolo
Copy link
Member

gziolo commented Jan 29, 2019

Yes, I see that something went wrong. We are still working on #13534 to provide docs which will cover the case when someone is using a fork of Gutenberg which is the case here. For the time being, I can share the reference to the GitHub docs: https://help.github.com/articles/syncing-a-fork/. We hope to have something more targeted for your use case later this week 👍

@gziolo
Copy link
Member

gziolo commented Jan 31, 2019

Sidebar Tutorial URL is wrong, should be .md

@manuelmeister It isn't part of this PR, so I would suggest opening another PR with a fix proposed.

@kadencethemes - can you check newly added docs about Git workflow: https://github.com/WordPress/gutenberg/blob/master/docs/contributors/git-workflow.md

If you can't fix it, it's fine to close this PR and open a follow up with only your changes applied on the up to date fork of Gutenberg.

@kadencewp
Copy link
Contributor Author

Well well, looks like there is hope for me :) I think I got it now.

@gziolo
Copy link
Member

gziolo commented Feb 1, 2019

Yeah, it’s fixed. Great success 🎉❤️

I hope that new documentation was helpful. If there is anything specific you feel we should add, let us know.

I will test this PR on Monday!

@kadencewp
Copy link
Contributor Author

I would suggest adding steps for users more accustomed to sourcetree.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

It tests well. Thank you for your contribution and fixing this bug. Much appreciated 💯

Also, thank you for being so patient as you had to wait a bit to get this merged and waste some times to refresh this PR 🥇

@gziolo
Copy link
Member

gziolo commented Feb 4, 2019

I would suggest adding steps for users more accustomed to sourcetree.

That would be good, right. I had to use GitHub desktop last week to update on of the PRs from the forked repository and merged it using code copied over to my own branch. It has just worked but it would be hard to explain how :)

@gziolo gziolo merged commit dc2c1f7 into WordPress:master Feb 4, 2019
daniloercoli added a commit that referenced this pull request Feb 5, 2019
…rnmobile/372-use-RichText-on-Title-block

* 'master' of https://github.com/WordPress/gutenberg: (22 commits)
  Make the modal title styling consistent (#13669)
  Disable navigation block for text mode. (#12185)
  Fix: Linting problem in modal example code (#13671)
  Add myself as a code owner to the annotations (#13672)
  Add more reviewers to CODEOWNERS.md file (#13667)
  Plugin: Remove jQuery heartbeat-to-hooks proxying (#13576)
  Workflow: Add repository CODEOWNERS file (#13604)
  Add a mobile minimum size for form fields (#13639)
  Update edit-save documentation  (#13578)
  Alt image setting (#13631)
  Fix: Allow years lower than 1970 in DateTime component. (#13602)
  Using addQueryArgs to generate Manage All Reusable Blocks link (#13653)
  Bump plugin version to 5.0.0-rc.1 (#13652)
  Update lodash to 4.17.10 (#13651)
  Refreshed PR (#9469)
  Set default values of the width and height input fields according to the actual image dimensions (#7687)
  12647 fix css color picker (#12747)
  Remove "we" from messages (#13644)
  Fix: Font size picker max width on mobile (#13264)
  Fix/issue 12501 menu item aria label
  ...
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
* Update css to fix padding with colorPicker

* Focus on to numbers

* Requested changes
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
* Update css to fix padding with colorPicker

* Focus on to numbers

* Requested changes
This was referenced Apr 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Design Feedback Needs general design feedback. [Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ColorPicker components: Input fields style issue
3 participants