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.

Sign up for GitHub

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

Jump to bottom

Business Hours: Update front-end of the description list #12393

Merged
merged 1 commit into from
May 17, 2019
Merged

Business Hours: Update front-end of the description list #12393

merged 1 commit into from
May 17, 2019

Conversation

thomasguillot
Copy link
Contributor

Reduce the size taken by the Business Hours on a page.

Changes proposed in this Pull Request:

  • Make sure parent element clears floats
  • Have the terms and descriptions on the same line
  • Terms elements should be bold to differentiate them from the descriptions (in case theme doesn't already style this)
  • Overwrite description's default margin
  • Add small right margin to terms

Testing instructions:

  • Add a Business Hours block to any post and page
  • Notice the new style in the editor when block is unfocussed
  • Notice the new style on the front-end

Before:

1 before
Twenty Nineteen / Shoreditch

After:

2 after
Twenty Nineteen / Shoreditch

Proposed changelog entry for your changes:

  • Business Hours: Update front-end of the description list

@thomasguillot
Business Hours: Update front-end of the description list
68a0199 
* Make sure parent element clears floats
* Have the terms and descriptions on the same line
* Terms elements should be bold to differenciate them from the descriptions
* Overwrite description's default margin
* Add small right margin to terms
@thomasguillot thomasguillot added [Status] Needs Review To request a review from Crew. Label will be renamed soon. [Pri] Normal [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack [Block] Business Hours labels May 16, 2019
@thomasguillot thomasguillot requested a review from a team as a code owner May 16, 2019 15:16
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello thomasguillot! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D28309-code before merging this PR. Thank you!

@jetpackbot
Copy link

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: June 4, 2019.
Scheduled code freeze: May 28, 2019

Generated by 🚫 dangerJS against 68a0199

@jeherve
Copy link
Member

jeherve commented May 16, 2019

Tests failed: D28309-code

Since you are adding new files, after you merge this PR you will need to add the new files to the diff manually to get the tests to pass.

@jeherve jeherve added this to the 7.4 milestone May 16, 2019
@jeherve jeherve added the [Status] Needs Design Review Design has been added. Needs a review! label May 16, 2019
jeherve
jeherve approved these changes May 16, 2019
View reviewed changes
Copy link
Member

@jeherve jeherve left a comment •
edited
Loading

Choose a reason for hiding this comment

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

It does look better imo. 👍 This should be good to merge once it's design approved.

@jeherve jeherve removed the [Status] Needs Review To request a review from Crew. Label will be renamed soon. label May 16, 2019
@shaunandrews
Copy link
Contributor

Much nicer without all that extraneous whitespace. Can you remove the repetitious "From" as well?

@jeherve jeherve added the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label May 16, 2019
@thomasguillot
Copy link
Contributor Author

I can have a look at the "From ... to ..." but probably in a different PR as it touches a different portion of the code

@jeherve jeherve removed the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label May 17, 2019
@jeherve
Copy link
Member

jeherve commented May 17, 2019

probably in a different PR as it touches a different portion of the code

In this case this should be good to merge. I've unblocked the PR so you should be able to merge, but as I mentioned above, you'll need a bit of manual work to port your changes over to WordPress.com in D28309-code this time:

Since you are adding new files, after you merge this PR you will need to add the new files to the diff manually to get the tests to pass.

@jeherve jeherve added the [Status] Ready to Merge Go ahead, you can push that green button! label May 17, 2019
@thomasguillot
Copy link
Contributor Author

Thank you @jeherve

@thomasguillot thomasguillot merged commit c53cf49 into master May 17, 2019
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels May 17, 2019
@thomasguillot thomasguillot deleted the update/business-hours-description-list branch May 17, 2019 09:24
jeherve added a commit that referenced this pull request May 17, 2019
@jeherve
Changelog: add #12393
37b1331
@thomasguillot thomasguillot mentioned this pull request May 17, 2019
Merged
jeherve added a commit that referenced this pull request May 23, 2019
@jeherve
Changelog: add #12393
22329d6
jeherve added a commit that referenced this pull request May 27, 2019
@kraftbj @jeherve
7.4 Changelog (#12304)
0fb4436 
* Kick off the changelog

* Add 7.3.1

* Update date and post link

* changelog: add #12219

* changelog: add #12170

* changelog: add #12184

* Changelog: add #12268

* Changelog: add #12081

* Changelog: add #12323

* Changelog: add #12204

* Changelog: add #12269

* Changelog: add #12332

* changelog: add #12339

* changelog: add #12209

* Changelog: add #12319

* Changelog: add #12357

* Changelog: add #12124

* Changelog: add #12373

* Changelog: add #12252

* Changelog: add #12383

* Changelog: add #12372

* changelog: add #12337

* Changelog: add #12290

* Changelog: add #12301

* Changelog: add #12061

* Testing list: add instructions for #12061

* Changelog: add #12393

* Update minimum supported version

See #12287

* Changelog: add #12406

* Testing list: add #12406

* Changelog: add #12277

* Changelog: add #12412

* Changelog: add #11318

* Changelog: add #12328

* Changelog: add #12425

* Changelog: add #12380

* Changelog: add #12428

* Changelog: add #12414

* Changelog: add #12395

* Changelog & Testing list: add #12416, #12417, #12418, and #12348

* changelog: add #12379

* Changelog: add #12341

* changelog: add #12444

* Changelog: add #12434

* Changelog: add #12454

* Changelog: add #12460

* Changelog: add #12463

* Changelog: add #12457

* Changelog / testing list: add #10333

* Changelog: add #12467


Co-authored-by: Jeremy Herve <jeremy@jeremy.hu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeherve jeherve jeherve approved these changes

@scruffian scruffian Awaiting requested review from scruffian

@iamtakashi iamtakashi Awaiting requested review from iamtakashi

Assignees

@thomasguillot thomasguillot

Labels
[Block] Business Hours [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack [Pri] Normal [Status] Needs Design Review Design has been added. Needs a review! Touches WP.com Files
Projects
None yet
Milestone
7.4
Development

Successfully merging this pull request may close these issues.
5 participants
@thomasguillot @matticbot @jetpackbot @jeherve @shaunandrews