Widgets editor: Display Widget Area's name and description in the sidebar #25943
Conversation
noisysocks
requested review from
adamziel,
draganescu,
kevin940726,
talldan,
tellthemachines and
youknowriad
as
code owners
|
@mapk: How's this look to you? |
|
Size Change: +2.48 kB (0%) Total Size: 1.19 MB
|
| Filename | Size | Change | |
|---|---|---|---|
build/a11y/index.js |
1.14 kB | 0 B | |
build/api-fetch/index.js |
3.35 kB | 0 B | |
build/block-directory/style-rtl.css |
943 B | 0 B | |
build/block-directory/style.css |
942 B | 0 B | |
build/block-library/theme-rtl.css |
741 B | 0 B | |
build/block-library/theme.css |
741 B | 0 B | |
build/block-serialization-default-parser/index.js |
1.78 kB | 0 B | |
build/block-serialization-spec-parser/index.js |
3.1 kB | 0 B | |
build/data-controls/index.js |
685 B | 0 B | |
build/deprecated/index.js |
772 B | 0 B | |
build/dom-ready/index.js |
568 B | 0 B | |
build/dom/index.js |
4.42 kB | 0 B | |
build/edit-navigation/style-rtl.css |
868 B | 0 B | |
build/edit-navigation/style.css |
871 B | 0 B | |
build/editor/style-rtl.css |
3.85 kB | 0 B | |
build/element/index.js |
4.45 kB | 0 B | |
build/escape-html/index.js |
734 B | 0 B | |
build/format-library/style-rtl.css |
547 B | 0 B | |
build/format-library/style.css |
548 B | 0 B | |
build/hooks/index.js |
1.74 kB | 0 B | |
build/is-shallow-equal/index.js |
710 B | 0 B | |
build/keycodes/index.js |
1.85 kB | 0 B | |
build/list-reusable-blocks/style-rtl.css |
476 B | 0 B | |
build/list-reusable-blocks/style.css |
476 B | 0 B | |
build/nux/style-rtl.css |
671 B | 0 B | |
build/nux/style.css |
668 B | 0 B | |
build/primitives/index.js |
1.34 kB | 0 B | |
build/priority-queue/index.js |
790 B | 0 B | |
build/redux-routine/index.js |
2.85 kB | 0 B | |
build/token-list/index.js |
1.24 kB | 0 B | |
build/url/index.js |
4.06 kB | 0 B | |
build/wordcount/index.js |
1.17 kB | 0 B |
| let description; | ||
| if ( ! selectedWidgetArea ) { | ||
| description = __( | ||
| 'Widget Areas are global parts in your site’s layout that can accept blocks. These vary by theme, but are typically parts like your Sidebar or Footer.' |
draganescu
Oct 8, 2020
Contributor
Is this text copied from somewhere?
Is this text copied from somewhere?
| ); | ||
| } else if ( selectedWidgetAreaId === 'wp_inactive_widgets' ) { | ||
| description = __( | ||
| 'Blocks in this Widget Area will not be displayed in your site.' |
draganescu
Oct 8, 2020
Contributor
The old use of inactive widgets was to "save" them for reuse, in order to not have to re-customize them by adding a new one. Perhaps we could explain this as well?
The old use of inactive widgets was to "save" them for reuse, in order to not have to re-customize them by adding a new one. Perhaps we could explain this as well?
|
Looks good!
A weird glitch happened when I first clicked away from the Inactive Widget area as you can see in the video, but seemed to work afterwards. |
…tifier for backwards compatibility
| const selectedWidgetArea = | ||
| selectedWidgetAreaId && | ||
| widgetAreas?.find( | ||
| ( widgetArea ) => widgetArea.id === selectedWidgetAreaId | ||
| ); |
Comment on lines
15
to
19
kevin940726
Oct 13, 2020
Member
Nitpick: Not sure about the performance impact, but maybe we would want to useMemo this so that we don't have to recalculate .find every time this component renders.
Nitpick: Not sure about the performance impact, but maybe we would want to useMemo this so that we don't have to recalculate .find every time this component renders.
| <BlockIcon icon={ blockDefault } /> | ||
| <div> | ||
| <p>{ description }</p> | ||
| { widgetAreas?.length === 0 && ( |
kevin940726
Oct 13, 2020
Member
What if widgetAreas is nullish? Do we want to display the empty message in this case?
I think that depends on whether we treat this state as a loading state or an error state 🤔 ?
What if widgetAreas is nullish? Do we want to display the empty message in this case?
I think that depends on whether we treat this state as a loading state or an error state
noisysocks
Oct 13, 2020
Author
Member
If we display the empty message when widgetAreas is nullish (! widgetAreas || widgetAreas.length === 0) then we'll see a flash of the empty message when the page loads, which is quite jarring. It's more common that there are widget areas so I think best to be optimistic about that.
If we display the empty message when widgetAreas is nullish (! widgetAreas || widgetAreas.length === 0) then we'll see a flash of the empty message when the page loads, which is quite jarring. It's more common that there are widget areas so I think best to be optimistic about that.
|
Looks good from testing and code review point of view. Not sure if you're still waiting for copy review, but that can always be a follow up. |
This was referenced Oct 14, 2020
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #25745.
Closes #24838.
Closes #25190.
This PR implements the mockup in #25745 (comment).
How to test