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

Add space to the Post Schedule date #3188

Merged
merged 7 commits into from
Nov 6, 2017
Merged

Add space to the Post Schedule date #3188

merged 7 commits into from
Nov 6, 2017

Conversation

Soean
Copy link
Member

@Soean Soean commented Oct 26, 2017

Description

Adds space between Post Schedule label and date
Resolve #3138

Screenshots:

Before:
before
before_date

After:
after
after_date

@youknowriad youknowriad added the Needs Design Feedback Needs general design feedback. label Oct 27, 2017
@mtias mtias added the [Feature] Document Settings Document settings experience label Oct 27, 2017
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

This looks like a good idea, but I think the styling should be applied to children of components-panel__row generally, not specific to the schedule toggle.

https://github.com/WordPress/gutenberg/blob/master/components/panel/style.scss

@codecov
Copy link

codecov bot commented Oct 30, 2017

Codecov Report

Merging #3188 into master will decrease coverage by 0.44%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3188      +/-   ##
==========================================
- Coverage   31.61%   31.17%   -0.45%     
==========================================
  Files         217      235      +18     
  Lines        6278     6534     +256     
  Branches     1116     1163      +47     
==========================================
+ Hits         1985     2037      +52     
- Misses       3609     3773     +164     
- Partials      684      724      +40
Impacted Files Coverage Δ
editor/sidebar/post-author/index.js 0% <0%> (-85%) ⬇️
editor/sidebar/post-pending-status/index.js 0% <0%> (-58.34%) ⬇️
blocks/library/gallery/gallery-image.js 0% <0%> (-55.56%) ⬇️
editor/sidebar/post-sticky/index.js 0% <0%> (-54.55%) ⬇️
blocks/library/gallery/index.js 36.84% <0%> (-25.66%) ⬇️
blocks/library/image/index.js 52.38% <0%> (-12.33%) ⬇️
components/navigable-menu/index.js 25.92% <0%> (-2.08%) ⬇️
editor/selectors.js 94.38% <0%> (-1.27%) ⬇️
blocks/library/gallery/block.js 10% <0%> (-1.12%) ⬇️
editor/utils/dom.js 15.2% <0%> (-0.91%) ⬇️
... and 86 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b2e25cd...1d8b4d0. Read the comment docs.

@Soean
Copy link
Member Author

Soean commented Oct 30, 2017

@aduth You are right, thats a better idea. I added the styling to .components-panel__row label and added the missing labels to post-schedule and post-visibility. I also added a id to the Dropdown component.

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

There are some issues with the labeling changes (which I'd also have recommended to create a separate pull request for):

  • With component patterns as in React, we don't want to assume that there is a single instance of an element on a page, and since id must be globally unique, 'post-visibility-selector' is not a good ID. For these cases we created the withInstanceId higher-order component to generate unique IDs to use as suffixes.
  • Since Dropdown expects the developer to implement renderToggle with a button, we don't need it to accept an id prop and rather can apply the id on the button returned in renderToggle (in PostSchedule and in PostVisibility).

@Soean
Copy link
Member Author

Soean commented Nov 2, 2017

I created a separate pull request for the labels: #3317, so this one has to be merged first.
This PR only adds space to the panel row labels and align the post schedule text.

Copy link
Member

@aduth aduth 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 👍

@aduth aduth merged commit 7bfdee7 into WordPress:master Nov 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Document Settings Document settings experience Needs Design Feedback Needs general design feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants