Search: Hide label when inserted in Navigation block #36026
Conversation
Mamaduka
added
[Block] Search
|
Size Change: +1.91 kB (0%) Total Size: 1.08 MB
ℹ️ View Unchanged
|
|
This is just beautiful. Here I'm inserting the search into navigation: Delightful. Since you're touching defaults, it feels worth it to also default to icon-only search button at least, and perhaps even to show that button inside, like so:
From my perspective, this just needs a code review! Thank you! |
|
Thanks for the great feedback, Joen. I've updated the contextual defaults as you suggested. CleanShot.2021-10-28.at.14.16.39.mp4 |
|
Looks just great to me, thanks so much! 🏅 |
| const hasChangedDefaults = useRef( false ); | ||
|
|
||
| useEffect( () => { | ||
| if ( hasChangedDefaults.current ) { |
There was a problem hiding this comment.
This only works when we first insert a Search block in Navigation. If we insert the block, save and reload the page, and then try changing the settings back to the defaults, they just change back 😬
We should probably also return if any of the settings have been edited.
Another potential edge case is already existing Search-within-Nav patterns: if the Search settings are the defaults, they'll be auto-changed, which might not be the desired outcome if the block has already been edited and saved before. I'm wondering if we should do this only for newly-inserted blocks - that would fix the issue of having edited settings reset after page reload too.
There was a problem hiding this comment.
I added few more checks in a895df9, to mitigate this. But I agree it's not most elegant solution.
I also tried using the wasBlockJustInserted selector, but it wasn't working for some reason. I will give this another go in a moment.
There was a problem hiding this comment.
@tellthemachines, thanks for the great feedback.
The wasBlockJustInserted selector doesn't work in the Navigation block context. It always returns false for the Search block. I think it's a bug that needs to be addressed separately.
Unfortunately, I'm not aware of another selector to check if the block is new.
As for the edge cases, I'm not sure. Should we consider it a bug we can live with while the wasBlockJustInserted selector issue is fixed?
I will leave that decision to you and @jasmussen.
There was a problem hiding this comment.
Hmm, this is tricky because wasBlockJustInserted only works for newly inserted blocks, not transformed blocks, and with how Navigation currently works chances are high that folks will be coming to the Search block via a transform.
To mitigate the issue with resetting the block to defaults after reload, we can store refs to the attribute values and return early in the effect if they change. I'm not sure there's an easy way to handle not changing existing blocks though.
Given Navigation is still experimental, perhaps there are not that many existing Navigations with Search blocks using all the default settings out there 😅
@jasmussen what are your thoughts on this?
There was a problem hiding this comment.
Apologies I missed the first ping!
I took the latest for a quick spin to just check things out, and I'm seeing the correctly-configured-for-nav search block work well:
I understand something happens if/when you reload? Or were you referring to existing navigation block that feature search?
There was a problem hiding this comment.
I was kindly explained that the problem might be that the action that sets the new defaults might fire every time the block loads, and therefore might reset back to the new defaults even if you want something else. Is that correct?
I wasn't actually able to reproduce this — it seems to continue working for me:
But more high level — while this is a really nice one that makes for a better navigation block experience, I wouldn't consider it crucial that this land in 5.9. If we can make it, nice. Otherwise, with 6.0 won't be that far behind!
There was a problem hiding this comment.
It's is true that the effect will run on every page reload. But the setAttribute will only fire if showLabel and buttonUseIcon attributes use the same value, so this is quite a specific situation.
@jasmussen to reproduce the issue, set button to use text and then enable label. You'll see that block will reset to the insertion state.
There was a problem hiding this comment.
Ah gotcha, so if you basically reset the navigation block to specifically look like the default state, it will reset on reload?
If we think we can fix the underlying bug in a followup and improve that, I'd be tempted to accept that tradeoff to get the feature landed.
There was a problem hiding this comment.
so if you basically reset the navigation block to specifically look like the default state, it will reset on reload?
That's correct.
There was a problem hiding this comment.
so if you basically reset the navigation block to specifically look like the default state, it will reset on reload?
Yes, but most importantly it will update existing Search blocks.
I made an alternative here: #36125, keeping some of this logic from George.
|
I was thinking about an alternative solution to this. A block variation is the most convenient method we have to preconfigure attributes. But we'd have to find a way to scope the variation only to the nav block. Could we consider an experimental |
|
@talldan, that's an interesting idea. I can try and explore it next week. |
|
I am closing this in favor of #36125. |
Description
Hide Search label when inserted inside the Navigation block.
Alternative to #35688.
Part of #31127.
How has this been tested?
Screenshots
CleanShot.2021-10-28.at.11.25.50.mp4
Checklist:
*.native.jsfiles for terms that need renaming or removal).