Skip to content

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

Merged
jeherve merged 3 commits intomasterfrom
fix/business-hours-block
Oct 21, 2019
Merged

Business hours block - fix styling in editor and front end#13728
jeherve merged 3 commits intomasterfrom
fix/business-hours-block

Conversation

@MichaelArestad
Copy link
Copy Markdown
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 Bug When a feature is broken and / or not performing as intended [Status] Needs Review This PR is ready for review. [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 a team and joanrho October 11, 2019 17:36
@MichaelArestad MichaelArestad self-assigned this Oct 11, 2019
@matticbot
Copy link
Copy Markdown
Contributor

matticbot commented Oct 11, 2019

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
Copy Markdown
Collaborator

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
Copy Markdown
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
Copy Markdown
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 need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Oct 14, 2019
@MichaelArestad
Copy link
Copy Markdown
Contributor Author

MichaelArestad commented Oct 17, 2019

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
Copy Markdown
Contributor

matticbot commented Oct 18, 2019

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

@MichaelArestad
Copy link
Copy Markdown
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 This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Oct 18, 2019
jeherve
jeherve requested changes Oct 18, 2019
View reviewed changes
Copy link
Copy Markdown
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 need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Oct 18, 2019
@MichaelArestad
Copy link
Copy Markdown
Contributor Author

MichaelArestad commented Oct 18, 2019

@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
Copy Markdown
Contributor

matticbot commented Oct 18, 2019

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

@MichaelArestad
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

MichaelArestad commented Oct 18, 2019

CC @thomasguillot

@jeherve jeherve added [Status] Needs Review This PR is ready for review. [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. [Status] Needs Review This PR is ready for review. labels Oct 21, 2019
jeherve
jeherve approved these changes Oct 21, 2019
View reviewed changes
Copy link
Copy Markdown
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

+1 more reviewer

@joanrho joanrho joanrho left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@MichaelArestad MichaelArestad

Labels

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

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