This repository has been archived by the owner on Feb 23, 2024. It is now read-only.
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.
Filter all products block by attribute #1127
Filter all products block by attribute #1127
Changes from 68 commits
47a36b2
dd1fe39
7f36fdf
63c3489
2a4bf48
3f2e1df
378d861
8941166
d1695c5
b92b99d
7d90236
98fae73
4aeaed1
dcb704c
7c26a00
2792bac
9e3ef5d
56f8d95
62711cf
505ce96
b02150c
b8a7780
e95e19c
d11aa29
18562a0
c5dfef4
9b955e8
21c244d
df96edb
8aa6811
0109336
24bba5c
0af54f9
e830631
a0dc217
97561a6
171fb41
18cb5d8
81b8abb
0ba0591
ae21f20
d9a691c
1a8e4af
b1a24d6
b4bcd39
9c14ae7
2270310
bc8dbfd
ca426f4
4a02e64
cdb3f09
0867c94
1d53d56
e250eba
fba12a2
7631fb8
4e1f250
dc492a2
7785207
53ba361
8c22ba0
129aebe
4a77711
0432ef7
607aeee
ab2351f
ded36b6
e87da8c
bbcebcb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fragment shouldn't be needed here, just put the
key
on the<li>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fragment wraps 2 sibling
<li>
elements so is needed here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right there is one Fragment that could remain. However there's a couple things:
The above points might not see important in the context of this particular usage, but it's pattern that could be more problematic in other contexts if it becomes a habit :). So I'm just promoting best practices here.
This code could get changed to:
Even better though, would be to handle the
renderedShowMore
andrenderedShowLess
outside of this memoized function. I'd combine therenderedShowMore
andrenderedShowLess
into one memoized function that spits out either of those or null based on the conditions. Then you could get rid of the last unnecessary fragment and end up with something like this:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm but by having the show more after the list items, if you tab through (with keyboard) when it expands you're at the bottom of the list rather than from the point of expansion.. that was my justification for having it appear inline. Originally I did it similar to you but only rendering visible list items. You were against that particular solution also.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 hmm... I see your point. I tried both behaviours and what you have definitely works better from an a11y pov. Let's roll with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we work around this by adding a key to the show more/show less li?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... I don't think so (not sure honestly), but regardless I think in this case any accessibility improvements make it worth potential extra rendering for the time being (until we can think of a better pattern).