DataViews: add initial "Side by side" prototype #55343
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
ellatrix
added
the
[Feature] DataViews
ellatrix
commented
Oct 13, 2023
| /> | ||
| </div> | ||
| </Page> | ||
| ) } |
There was a problem hiding this comment.
Ideally this is part of the view component. Maybe a slot?
There was a problem hiding this comment.
Why a slot? Why it's not bundled in the view from the start ?
There was a problem hiding this comment.
Because it needs to be rendered into a separate Page component?
There was a problem hiding this comment.
Ok, so for me, it's either too early for this particular view to be implemented or we need to redesign it a bit. In the sense that instead of it being a separate page/frame, I think it should be within the same frame and just separated from the list with a light border: most applications that use this pattern do this (Mac OS notes app is the simplest example).
If it needs to breakout of the "Page" component, I think it introduces too much complexity that we don't need at this stage of development for a small outcome. It starts to raise the question of whether it's a "layout" or something else more global. (The layout being just the left sidebar)
cc @SaxonF
There was a problem hiding this comment.
I disagree with it being a small outcome
There was a problem hiding this comment.
The space in between is just styling at the end of the day. The bigger reason it's separate is because there should be no "Pages" heading at the top.
There was a problem hiding this comment.
I understand Riad's concern. I too was looking at the whole picture in terms of:
- A consumer, like PagePages, provides view settings, actions, and a bunch of entity records.
- The rest is mostly left to DataViews: mostly rendering lists, but potentially rendering some details (for now) and doing other handling on the entity records.
- The consumer mostly reacts to changes in the high-level data (e.g. change query args for the entities).
I think the line between what is abstracted by DataViews and what is up for the consumer to handle is a line likely to change as we progress, but for now I also feel we should intentionally restrict our model. Ella, do you feel it's incompatible to have the Editor inside ViewSideBySide?
There was a problem hiding this comment.
@mcsf I don't know how you see that work? This is the view component area? How do you fit in a preview?
There was a problem hiding this comment.
Other than a slot, not sure what solutions you all have in mind?
There was a problem hiding this comment.
Yeah, I'm not sure now. And Slot might be overkill.
|
Size Change: +253 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
|
I wanted to share my current mental modal for this one here:
So in pseudo code something like this: count [ selection, setSelection ] = useState( [ 1, 2 ] ); // Holds the currently selected items.
<Page>
<DataViews {...regularProps} selection={selection} onChangeSelection={setSelection} />
</Page>
{ selection.lenght === 1 && view.type = "list-v2" && ( <Page>
<Preview selection={selection} />
</Page> ) } |
ellatrix
force-pushed
the
try/pages-split
branch
from
b42a56e
to
5fcec1c
Compare
youknowriad
reviewed
Oct 17, 2023
|
|
||
| // To do: convert to view type registry. | ||
| export const viewTypeSupportsMap = { | ||
| list: {}, |
There was a problem hiding this comment.
I feel we should rename this one "table" and the new one "list"
There was a problem hiding this comment.
I don't understand what you mean here. You named it list and grid?
There was a problem hiding this comment.
I think that makes some sense
- rename "list" to "table": totally on board; as we expand on the feature set of ViewList it will work and more more like a table
- rename "side-by-side" to "list": not as clear, but functionally the left-hand side is a list
youknowriad
reviewed
Oct 17, 2023
| @@ -117,6 +120,15 @@ export default function PagePages() { | |||
| postType: page.type, | |||
| canvas: 'edit', | |||
| } } | |||
| onClick={ ( event ) => { | |||
There was a problem hiding this comment.
This shouldn't be in the "fields" config IMO and also "fields" config shouldn't depend on local state here, it should only use things passed as props. This of this "render" function as the "edit" function of blocks. It might be defined in a very different file/package...
ellatrix
force-pushed
the
try/pages-split
branch
from
808a214
to
d95e570
Compare
mcsf
approved these changes
Oct 19, 2023
| import Editor from '../editor'; | ||
| import { useInitEditedEntity } from '../sync-state-with-url/use-init-edited-entity-from-url'; | ||
|
|
||
| export default ( { postType, postId } ) => { |
There was a problem hiding this comment.
What can we name this component?
There was a problem hiding this comment.
Renamed it to SideEditor
ellatrix
force-pushed
the
try/pages-split
branch
from
d95e570
to
3518722
Compare
ellatrix
force-pushed
the
try/pages-split
branch
from
3518722
to
c45bc91
Compare
oandregal
changed the title
oandregal
added
[Type] Experimental
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.
Part of #55083
What?
Adds another view type called side by side, allowing you to preview the page.
To do:
Not sure about the naming btw. Side by side sounds like a comparison.
Why?
Mock up by @SaxonF
How?
We can reuse the
Editorcomponent.Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast