Skip to content
This repository has been archived by the owner on Jul 12, 2024. It is now read-only.

Fix wc-admin nav styles after GB 11.6.0 changes #7771

Merged
merged 7 commits into from
Oct 12, 2021
Merged

Conversation

lsl
Copy link
Contributor

@lsl lsl commented Oct 8, 2021

Fixes #7683

A recent GB refactor broke the styling in wc-admin, this PR introduces some of the same changes that were used in that PR.

Detailed test instructions:

Screenshots

Before 11.6.0
Screenshot(37)

Before this change
Screenshot(41)

After this change
Screenshot(42)

Copy link
Contributor

@joshuatf joshuatf left a comment

Choose a reason for hiding this comment

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

Thank you very much for the fix-up, @lsl! Left a couple comments in the code.

Could you also add Fixes #7683 to the original post above so this links to #7683?

@@ -71,4 +71,40 @@
padding: $gap $gap-smaller $gap-small $gap-smaller;
}
}

/*
* Override Gutenberg changes to the navigation component introduced in 11.6.0 by https://github.com/WordPress/gutenberg/pull/34885
Copy link
Contributor

Choose a reason for hiding this comment

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

Are the changes in Gutenberg temporary or permanent? If we don't see them going away anytime soon, we can probably remove this comment and merge these styles with the above .components-navigation__menu selector.

It's hard to tell from the discussion in WordPress/gutenberg#34885 but it seems like they want to move to the G2 navigation, but updated the styles in this nav component anyway. I have some doubts if the styling will be reverted back even if they migrate to g2.

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 don't know enough about these components to comment with any kind of authority but yea, I doubt these are temporary which is why I opted to keep them out of the "temporary" section above. Wrt merging in with the rest of the code I think its worth keeping overrides distinctly separate but maybe this whole section is overrides?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this section is overriding GB components. I went ahead and merged these selectors/styles together. Thank you very much for the changes, @lsl!

Co-authored-by: Joshua T Flowers <[email protected]>
@mattsherman mattsherman added this to the 2.8.0 milestone Oct 11, 2021
@mattsherman
Copy link
Contributor

@lsl @joshuatf What's the status of this one? Is it being worked on still, or is it ready for re-review? It is unclear to me, given the lack of any status: label. Thanks!

@joshuatf
Copy link
Contributor

@mattsherman I'm not 100% sure, but I think @lsl made all the changes except merging these with existing selectors. I can do that today so we can get this merged and cherry picked.

@joshuatf
Copy link
Contributor

Hey @mattsherman - @lsl's other changes looked good. I merged with our existing selectors in 2bdbb5b and also fixed up a back button sizing issue I noticed in e882997.

This could use some 👀 before cherry picking just to sanity check my additions.

Copy link
Collaborator

@psealock psealock left a comment

Choose a reason for hiding this comment

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

I tested this with the problematic version of Gutenberg and previous versions. Either case is looking good and I didn't notice any style issues. Code changes look sane as well.

I saw some whitespace issues and fixed them in b7a2990 by just letting WP Prettier do its thing. I'm not sure how that escaped the pre-commit hooks which I thought handled this sort of thing 🤷🏽‍♀️

Many thanks @lsl for kicking this off and @joshuatf for following up 🥇

@ilyasfoo
Copy link
Contributor

Merging this as it's reviewed sufficiently. Thanks again, folks!

@ilyasfoo ilyasfoo merged commit 897f6dc into main Oct 12, 2021
@ilyasfoo ilyasfoo deleted the fix/nav-style branch October 12, 2021 10:28
mattsherman pushed a commit that referenced this pull request Oct 12, 2021
* Fix wc-admin nav styles after GB 11.6.0 changes

* log

* Update changelogs/fix-nav-style

Co-authored-by: Joshua T Flowers <[email protected]>

* Hover fix for back button

* Merge styles with existing

* Fix back button size

* fix whitespace issues

Co-authored-by: Joshua T Flowers <[email protected]>
Co-authored-by: Paul Sealock <[email protected]>
ObliviousHarmony pushed a commit to woocommerce/woocommerce that referenced this pull request Mar 18, 2022
…rce-admin#7771)

* Fix wc-admin nav styles after GB 11.6.0 changes

* log

* Update changelogs/fix-nav-style

Co-authored-by: Joshua T Flowers <[email protected]>

* Hover fix for back button

* Merge styles with existing

* Fix back button size

* fix whitespace issues

Co-authored-by: Joshua T Flowers <[email protected]>
Co-authored-by: Paul Sealock <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Look into breaking Navigation style updates
5 participants