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 block - fix styling in editor and front end #13728

Merged
merged 3 commits into from
Oct 21, 2019
Merged

Business hours block - fix styling in editor and front end #13728

merged 3 commits into from
Oct 21, 2019

Conversation

MichaelArestad
Copy link
Contributor

@MichaelArestad MichaelArestad commented Oct 11, 2019 •
edited by gravityrail
Loading

Fixes #13780

Fixes display of business hours. It was buggy on Twenty Twenty and this not only resolves that display issue, but hardens it in the future (probably) from breakage.

Changes proposed in this Pull Request:

  • Changes the way the dd and dt were positioned. No longer relies on floats.

Before

image

image

After

image

image

Is this a new feature or does it add/remove features to an existing part of Jetpack?

  • Fixes existing feature.

Testing instructions:

  • Install Twenty Twenty
  • Go to post-new.php
  • Add a business hours block
  • Make sure to check the preview (top inserter only)
  • Publish and check the front end

Proposed changelog entry for your changes:

  • Might need a changelog entry since front end styles and html were slightly modified?

@MichaelArestad MichaelArestad added [Type] Bug When a feature is broken and / or not performing as intended [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack [Block] Business Hours labels Oct 11, 2019
@MichaelArestad MichaelArestad added this to the 7.9 milestone Oct 11, 2019
@MichaelArestad MichaelArestad requested review from joanrho and a team October 11, 2019 17:36
@MichaelArestad MichaelArestad self-assigned this Oct 11, 2019
@matticbot
Copy link
Contributor

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

@jetpackbot
Copy link

jetpackbot commented Oct 11, 2019 •
edited
Loading

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: November 5, 2019.
Scheduled code freeze: October 29, 2019

Generated by 🚫 dangerJS against 88250bc

@joanrho
Copy link
Contributor

joanrho commented Oct 11, 2019 •
edited
Loading

Just tested and I can verify that it looks like your "After" screenshots, but there's a slight discrepancy between the text leading in the Editor view and published post/page view that still feels a bit off to me. This isn't different from the same visual discrepancy (between Editor view and published post/page view) from other blocks like Contact Form though, so I'm going to go ahead and approve this.

joanrho
joanrho previously approved these changes Oct 11, 2019
View reviewed changes
jeherve
jeherve requested changes Oct 14, 2019
View reviewed changes
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This seems to break the layout when you have multiple opening hours within a single day.

Before:

  • In the editor:
    image
  • On the frontend:

screenshot 2019-10-14 at 10 07 15

After

  • In the editor:
    image

  • On the frontend:
    image

@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Oct 14, 2019
@MichaelArestad
Copy link
Contributor Author

Hmm. Good catch. I'm wondering if we need to rethink styles and html a little bit. Will see what I can do.

@matticbot
Copy link
Contributor

MichaelArestad, Your synced wpcom patch D33939-code has been updated.

@MichaelArestad
Copy link
Contributor Author

MichaelArestad commented Oct 18, 2019 •
edited
Loading

@jeherve Made some changes. Can you check my code?

(I'll be messing a little more with the styles tomorrow, but this might be good enough)

As of the latest commit:

Editor preview:

image

Site preview:

image

@MichaelArestad MichaelArestad added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Oct 18, 2019
jeherve
jeherve requested changes Oct 18, 2019
View reviewed changes
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

It does work better, although I think we're missing a bottom margin below the list now:
image

@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Oct 18, 2019
@MichaelArestad
Copy link
Contributor Author

@jeherve I didn't make any changes that reduce the bottom margin. I think the change I made actually made it slightly larger. I could add a margin. @joanrho what do you think?

@matticbot
Copy link
Contributor

MichaelArestad, Your synced wpcom patch D33939-code has been updated.

@MichaelArestad
Copy link
Contributor Author

MichaelArestad commented Oct 18, 2019 •
edited
Loading

Made more changes to match original mocks:

Editor

image

Rendered

image

Mobile

image

Original figma design for reference:

image

CC @joanrho

@MichaelArestad
Copy link
Contributor Author

CC @thomasguillot

@jeherve jeherve added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Oct 21, 2019
jeherve
jeherve approved these changes Oct 21, 2019
View reviewed changes
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This seems to work well for me. Merging.

@jeherve jeherve merged commit 0e8bfa0 into master Oct 21, 2019
@jeherve jeherve deleted the fix/business-hours-block branch October 21, 2019 09:43
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Oct 21, 2019
jeherve added a commit that referenced this pull request Oct 23, 2019
@jeherve
Changelog: add #13728
a98c518
jeherve added a commit that referenced this pull request Oct 29, 2019
@jeherve
7.9: Changelog (#13608)
6cd1621 
* 7.9: Changelog

* Update version number

* Update stable tag and tested up to

* Changelog: add #13530

* changelog: add #13578

* Changelog: add #13598

* Changelog: add entry for numerous block preview changes

* Changelog: add #13599

* changelog: add #13541

* Changelog: add #13542

* Changelog: add #13331

* Changelog: add #13558

* Changelog: add #13409

* Changelog: add #13582

* Changelog: add #13600

* Changelog: add #13601

* Changelog: add #13595

* Changelog: add #12695

* Changelog: add #13009

* Changelog: add #13649

* Changelog: add #13450

* Changelog: add #13507

* Changelog: add #13658

* Changelog: add #13687

* changelog: add #13683

* Changelog: add #9323

* Changelog: add #13681

* Fix typos in readme

* Add link to WordPress Beta Tester plugin

* Changelog: add #13630

* Changelog: add #13695

* Changelog: add #13659

* Changelog: add #13716

* Changelog: add #13664

* Changelog: add #13682

* Changelog: add #13362

* Changelog: add #13563

* Add testing list for #13563

* Changelog: add #13735

* Changelog: add #13752

* Changelog: add #13624

* Changelog: add #13756

* Changelog: add #13745

* Changelog: add #13728

* Changelog: add #13779

* Changelog: add #13699

* Changelog: add #13804

* Changelog: add #13761

* Changelog: add #13637

* Changelog: add #13517

* Changelog: add #13521

* Changelog: add #13729

* Testing list: add testing instructions for #13729

* Changelog: add sync changes

* Changelog: add #13807

* Changelog: add #13654

* Changelog: add #13795

* Changelog: add #13801

* Changelog: add #13818

* Changelog: add #13725

* Changelog: add #13831

* Changelog: add #13516

* Testing list: add Twenty Twenty instructions

* Changelog: add #13799

* Changelog: add #13805

* Changelog: add #13688

* Changelog: add #13830
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

@joanrho joanrho joanrho left review comments

Assignees

@MichaelArestad MichaelArestad

Labels
[Block] Business Hours [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack Touches WP.com Files [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Milestone
7.9
Development

Successfully merging this pull request may close these issues.

Business hours block formatting broken on twentytwenty theme
5 participants
@MichaelArestad @matticbot @jetpackbot @joanrho @jeherve