Switch tabs after successful upload of fonts #55975
Open
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
madhusudhand
changed the title
|
Size Change: +41 B (0%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
|
This definitely works as expected. Tab behavior is what I expected. 2 things to note: The 'success' notification isn't visible because of the switch. Which I believe is OK since the switch kind of demonstrates the success. Error message are seen and that's good. It might be nice to show the success on the Library Tab. The "Install Fonts" tab doesn't behave the same way and I believe it probably should. |
alexstine
reviewed
Nov 9, 2023
There was a problem hiding this comment.
I really don't like the accessibility situation here. It would be one thing if the upload page was separate but I am not sure switching tabs spells out success clearly enough for non-sighted users. Where would focus land? On the newly uploaded font or does it just get left out in the void? I need additional testing on this before this gets merged.
|
@jasmussen Could you share your thoughts on this? |
|
Thanks for the ping, thanks for exploring. To explain the fuller context, I'll rewind a bit. The fonts library tab should ideally be a silo of fonts, a bit like how the media library. Right now it's doing a bit of double duty, also defining whether a font is active or not. This is exacerbated a bit by the fact that when you upload a font, that font is inactive, it's simply added to the library. After upload, you have to actually go to the font in the library to activate it. Before discussing options, a quick review of what exists in trunk at the moment:
All of the above is shown in this GIF:
A few issues with that:
Now, reviewing this branch, what happens when you upload a font file is that you are immediately taken to the library tab. But to Alex' point, there's no confirmation of what actually happened. I appreciate the PR, but the original intent was to take you to the font page, i.e. Library > [Just-uploaded font name]. The idea here being, that just uploading the font does little, you have to activate it too, that's the obvious next step. This is shown in the following GIF:
Question: would that original intent be okay? Upon upload, redirect to the newly uploaded font so that you can activate it? |
madhusudhand
force-pushed
the
fix/54779
branch
from
5c53e02
to
740f895
Compare
|
Flaky tests detected in 740f895. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7208714375
|
|
@jasmussen Thanks for reviewing. I have addressed the suggested feedback.
Enabled Notices to be dismissible. They stay in the UI until dismissed by the user. 
Yes. Installed font is automatically selected in the library tab after the successful upload.
Focus would go to Library tab after uploading the font. 
However the accessibility situation isn't great with in the library content panel. I would like to keep it in a different issue to refine further. |
Yeah, I know. The true problem becomes, if accessibility is split out from this PR, it may get left behind for another Core release. It's just the way it happens a lot of the time. 😞 I'll try to get this tested at some point. |
mikachan
added
[Type] Enhancement
Code is taken out from this PR and addressed separately here #58178 Rebased with #57181 for @alexstine Following is the latest situation.
font-tab-switching.mp4 |
madhusudhand
force-pushed
the
fix/54779
branch
from
740f895
to
9bdb4a4
Compare
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 change replaces
TabPanelwithTabscomponent (which is a controlled component), and switches to Library tab (first tab) after successfully uploading the font from "Upload" tab.Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast
References:
Fixes: #54779
Tabs component implementation: #53960