Fix warning when a template calls a template area twice #54861
Merged
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
MaggieCabrera
added
the
[Type] Bug
MaggieCabrera
requested review from
draganescu,
scruffian,
getdave,
jeryj,
ajlende and
carolinan
|
Size Change: +585 B (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
|
@MaggieCabrera, I've got an alternative fix for this - #54863. I was just typing the description and noticed your PR. |
yours feels like more sensible approach. I'm assuming we want the same template part to show up twice then? |
It sort of makes sense to me, if we assume the "Areas" component is mini-"List View". @ramonjd or @kevin940726 might have a definitive answer here as they build the components. |
I see this Areas component more as a shortcuts view to dive into the available template parts, not a detailed list view. I think it's fine to not render the same template part twice. |
|
Indifferent of wether we want to show the template part twice or not, I wonder if the solution here produces the expected resutl: the function should return the complete list, or be named to contain "unique" and make sure we always want it to return unique items. If we want to do what @richtabor says I think a combo of @Mamaduka 's pr to safeguard in the future (using a better key) and also filter for uniqueness closer to the rendered component not directly in the selector? |
This is a good point. It'd be great to be able to add a unique title or description to theme entities like this, but then we'd have to save them somewhere. Maybe we could build one, e.g., extrapolate some information based on where the template area appears in the template. 🤷🏻 Or we could provide a count in some sort of consolidated view 
|
|
Thanks for the quick PR @MaggieCabrera I've just tested and approved #54863 I think it might be useful to know as a consumer of the selectors how many duplicates there are, so maybe we could leave it late (in the UI components) to decide whether to filter them or not (?) That's just my opinion - open to alternatives. |
|
Since #54863 is merged we may want to change this PR to simply be about the visual update @richtabor proposed of showing same template part once. |
scruffian
approved these changes
Sep 28, 2023
|
I don’t understand. Did we agree that it was okay to change public function signature? I don’t mind change but things like this should be communicated. Edit: Sorry, I just realized that this came out as an angry comment. It wasn't my intention. But I think @draganescu has a valid point (#54861 (comment)). |
scruffian
added a commit
that referenced
this pull request
Sep 29, 2023
…)" This reverts commit f637b37.
|
Sorry I didn't fully grok @draganescu's comment. Let's revert this and move the filter closer to the rendered component. |
|
Thank you, @scruffian! |
scruffian
added a commit
that referenced
this pull request
Sep 29, 2023
|
Another attempt at this here: #54942 |
Thank you for pointing out my mistake :) |
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.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
This PR removes duplicate template areas from the sidebar when in Site View.
The list of areas is based on the template parts used on the page, but we may use some template parts twice.
Why?
We only want to display these template areas and corresponding parts once in the sidebar even if they are used multiple times.
How?
By making sure that
getFilteredTemplatePartBlocksonly returns one instance of eachtemplateId, making them uniqueScreenshot