Add: UI to switch between dataviews and create new ones. #55497
Closed
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
|
Warning: Type of PR label error To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. |
|
Size Change: +600 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 03c7a09. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6695479804
|
jorgefilipecosta
force-pushed
the
add/ui-to-switch-between-views-and-create-new-ones
branch
from
6963934
to
03c7a09
Compare
youknowriad
reviewed
Oct 31, 2023
| @@ -25,7 +25,7 @@ const { useLocation } = unlock( routerPrivateApis ); | |||
| // (entity request and useEffect to update the view). | |||
There was a problem hiding this comment.
Why we have this provider component? What's its purpose?
There was a problem hiding this comment.
Share the state between the sidebar and the main area.
youknowriad
reviewed
Oct 31, 2023
| // TODO: add views UI. | ||
| return null; | ||
| const { dataviews, type, taxonomyId, currentViewId, setCurrentViewId } = | ||
| useContext( DataviewsContext ); |
There was a problem hiding this comment.
Why are we using a context for this?
youknowriad
reviewed
Oct 31, 2023
| function AddNewItem( { type, taxonomyId } ) { | ||
| const { saveEntityRecord } = useDispatch( coreStore ); | ||
|
|
||
| const [ isAdding, setIsAdding ] = useState( false ); |
There was a problem hiding this comment.
Why do we need this boolean in addition to dataViewIsSaving?
There was a problem hiding this comment.
Oh I guess, this one controls the visibility of the modal.
There was a problem hiding this comment.
Exactly control modal visibility.
youknowriad
reviewed
Oct 31, 2023
| ), | ||
| }; | ||
| }, [] ); | ||
| useEffect( () => { |
There was a problem hiding this comment.
Can we avoid this effect by using "await" in the event handler instead?
Also, why are we "saving" on rename automatically, can't we just "edit" the entity and let the user save later. I guess this is more of a design question (and should probably match what we do in all screens). cc @jameskoster @SaxonF
There was a problem hiding this comment.
This is not for renaming, this is to create a new entity, the user provides the name of the entity, and we need to create it so then the user can change that entity that did not exist before.
There was a problem hiding this comment.
Saxon has thought more about this so I'll defer to him, but my initial reaction is to wonder why we need the user to actively save views. Could they be auto-saved instead? Requiring a save makes the UX a bit long-winded, and the mixture of site + admin details in the save modal feels a little off to me.
There was a problem hiding this comment.
Agree with Riad here. It will be a bit cleaner with await, no? Something similar to AddCustomGenericTemplateModalContent and keep isCreating with local state.
youknowriad
reviewed
Oct 31, 2023
| setIsAdding( false ); | ||
| } | ||
| }, [ dataViewIsSaving ] ); | ||
| useEffect( () => { |
There was a problem hiding this comment.
Can we avoid this effect by just calling setTitle directly when needed? (Yeah, if we can avoid useEffect, we'll avoid subtle bugs :P )
There was a problem hiding this comment.
We can do that, but in this case, every time the modal closes, I want to clean the title. It seemed better to have the effect that calls setTitle in every place that closes the modal. This seemed more future-proof as soon we may have a place that closes the modal and we forget to reset the title. But either option is fine I will remove the effect,
There was a problem hiding this comment.
Maybe extract the Modal code in a different component so it will keep its state and reset when closed/reopened?
|
I assume it's on the todo list, but just to be sure; it'd be nice to make the menu items match others in the sidebar. It looks like the ellipsis button is making the view menu items too large, but if we use the 24px variant that seems to fix things. We should probably separate the view menu items from the + New view button a little. Just some space might be worth a try. The + New view button should be the same color as other menu items as well. Currently it's possible to create views with the same name as exiting views (or with no name at all), should we validate on creation? I appreciate it's beyond the scope of this PR, but leaving a note here since creating a grid view results in the It might be better to call that layout "Gallery" and use something like the image icon. "Gallery" is a bit more descriptive, particularly for mobile where a single column is the more likely layout. Additionally, the icon in the "View" button should match the chosen layout, similar to the menu item. |
|
Supersed by #55740. |
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.
This PR adds the UI to switch between data views and to create new ones.
To keep the PR size manageable the UI to detele and the UI to rename views will be proposed as a follow-up.
Screenshots