This repository has been archived by the owner on Feb 23, 2024. It is now read-only.
Limit number of visible incompatible extensions in sidebar notice #11972
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nielslange
added
type: enhancement
woocommercebot
requested review from
a team and
opr
and removed request for
a team
|
The release ZIP for this PR is accessible via: Script Dependencies ReportThere is no changed script dependency between this branch and trunk. This comment was automatically generated by the TypeScript Errors Report
🎉 🎉 This PR does not introduce new TS errors. |
|
Size Change: +554 B (0%) Total Size: 1.53 MB
ℹ️ View Unchanged
|
…atible-extensions-in-sidebar-notice
nerrad
added
focus: cart
opr
suggested changes
Nov 30, 2023
There was a problem hiding this comment.
@nielslange this PR is working as expected, I have made an inline suggestion, and I do have another suggestion about the icon. Maybe moving the chevron down a few pixels when it's expanded would be better, toggling between open/closed it looks like it is not aligned correctly with the text when it's expanded.
assets/js/editor-components/incompatible-extension-notice/index.tsx
Outdated
Show resolved
Hide resolved
Thanks for your review, @opr. Regarding the icon, I tried to mimic the behaviour of Gutenberg. That said, I just applied your suggestion and moved the chevron 2px down when it's opened. It looks indeed better, but it looks slightly different from the behavior of the Gutenberg chevron. I'm looking forward to hearing your thoughts on that. |
opr
suggested changes
Nov 30, 2023
This reverts commit ff51424.
Thanks, @opr. I just reverted the chevron commit and all tests passed. |
nielslange
deleted the
update/11557-limit-number-of-visible-incompatible-extensions-in-sidebar-notice
branch
github-actions
bot
added
status: ready to merge
and removed
status: ready to merge
labels
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What
Fixes #11557
This PR limits the display of incompatible extensions to two, with the option to view additional ones in a dropdown menu. Instead of introducing an according component to show remaining incompatible extensions, I decided to use the HTML element
<details>.The proposed design in #11557 shows that the
<details>element does not have the marker on the left-hand side, but a chevron on the right-hand side. For consistency-reasons, I used the chevron from@wordpress/icons, which is used to collapse various sidebar sections. For Safari, I had to introduce a separate definition to remove the marker (see review below).In #11557 (comment), @elizaan36 mentioned that the link color should be changed to gray and be underlined. While cross-browser testing, I noticed that Safari does not allow adding
text-decorationto the<summary>element. Therefore, I introduced a<span>to ensure that the link is underlined in all browsers.Why
This PR enhances the user experience by creating a more clear UI. It also prevents overwhelming merchants by showing too many incompatible extensions by default.
Testing Instructions
Please consider any edge cases this change may have, and also other areas of the product this may impact.
Note
Things to keey an eye on while testing:
Test with 2 extensions
Test with 3 extensions
1 more incompatibility.Test with 10 extensions
8 more incompatibilities.Screenshots or screencast
Before and after
Cross-browser compatibility
Note
WooCommerce Visibility
Required:
Checklist
Required:
[type]label or a[skip-changelog]label.Conditional:
[skip-changelog]label is not present).Changelog