Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

DataViews: add initial "Side by side" prototype #55343

Merged
merged 2 commits into from
Nov 3, 2023
Merged

DataViews: add initial "Side by side" prototype #55343

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
222b484
Add side by side
ellatrix Oct 20, 2023
c45bc91
Rename side editor
ellatrix Oct 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion packages/edit-site/src/components/dataviews/dataviews.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,22 @@ import ViewActions from './view-actions';
import Filters from './filters';
import Search from './search';
import { ViewGrid } from './view-grid';
import { ViewSideBySide } from './view-side-by-side';

// To do: convert to view type registry.
export const viewTypeSupportsMap = {
list: {},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel we should rename this one "table" and the new one "list"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what you mean here. You named it list and grid?

Copy link
Contributor

@mcsf mcsf Oct 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

grid: {},
'side-by-side': {
preview: true,
},
};

const viewTypeMap = {
list: ViewList,
grid: ViewGrid,
'side-by-side': ViewSideBySide,
};

export default function DataViews( {
view,
Expand All @@ -28,7 +44,7 @@ export default function DataViews( {
isLoading = false,
paginationInfo,
} ) {
const ViewComponent = view.type === 'list' ? ViewList : ViewGrid;
const ViewComponent = viewTypeMap[ view.type ];
const _fields = useMemo( () => {
return fields.map( ( field ) => ( {
...field,
Expand Down
2 changes: 1 addition & 1 deletion packages/edit-site/src/components/dataviews/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
export { default as DataViews } from './dataviews';
export { default as DataViews, viewTypeSupportsMap } from './dataviews';
4 changes: 4 additions & 0 deletions packages/edit-site/src/components/dataviews/view-actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,10 @@ const availableViews = [
id: 'grid',
label: __( 'Grid' ),
},
{
id: 'side-by-side',
label: __( 'Side by side' ),
},
];

function ViewTypeMenu( { view, onChangeView } ) {
Expand Down
9 changes: 9 additions & 0 deletions packages/edit-site/src/components/dataviews/view-side-by-side.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
/**
* Internal dependencies
*/
import ViewList from './view-list';

export function ViewSideBySide( props ) {
// To do: change to email-like preview list.
return <ViewList { ...props } />;
}
7 changes: 6 additions & 1 deletion packages/edit-site/src/components/layout/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,11 +157,16 @@
}

// This shouldn't be necessary (we should have a way to say that a skeletton is relative
.edit-site-layout__canvas .interface-interface-skeleton {
.edit-site-layout__canvas .interface-interface-skeleton,
.edit-site-page-pages-preview .interface-interface-skeleton {
position: relative !important;
min-height: 100% !important;
}

.edit-site-page-pages-preview {
height: 100%;
}

.edit-site-layout__view-mode-toggle.components-button {
position: relative;
color: $white;
Expand Down
70 changes: 54 additions & 16 deletions packages/edit-site/src/components/page-pages/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import { privateApis as routerPrivateApis } from '@wordpress/router';
*/
import Page from '../page';
import Link from '../routes/link';
import { DataViews } from '../dataviews';
import { DataViews, viewTypeSupportsMap } from '../dataviews';
import { default as DEFAULT_VIEWS } from './default-views';
import {
useTrashPostAction,
Expand All @@ -27,6 +27,7 @@ import {
viewPostAction,
useEditPostAction,
} from '../actions';
import SideEditor from './side-editor';
import Media from '../media';
import { unlock } from '../../lock-unlock';
const { useLocation } = unlock( routerPrivateApis );
Expand All @@ -44,6 +45,8 @@ const defaultConfigPerViewType = {
export const DEFAULT_STATUSES = 'draft,future,pending,private,publish'; // All statuses but 'trash'.

export default function PagePages() {
const postType = 'page';
const [ selection, setSelection ] = useState( [] );
const {
params: { path, activeView = 'all' },
} = useLocation();
Expand Down Expand Up @@ -99,7 +102,7 @@ export default function PagePages() {
isResolving: isLoadingPages,
totalItems,
totalPages,
} = useEntityRecords( 'postType', 'page', queryArgs );
} = useEntityRecords( 'postType', postType, queryArgs );

const { records: authors, isResolving: isLoadingAuthors } =
useEntityRecords( 'root', 'user' );
Expand Down Expand Up @@ -136,7 +139,7 @@ export default function PagePages() {
header: __( 'Title' ),
id: 'title',
getValue: ( { item } ) => item.title?.rendered || item.slug,
render: ( { item } ) => {
render: ( { item, view: { type } } ) => {
return (
<VStack spacing={ 1 }>
<Heading as="h3" level={ 5 }>
Expand All @@ -146,6 +149,14 @@ export default function PagePages() {
postType: item.type,
canvas: 'edit',
} }
onClick={ ( event ) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed 😊

if (
viewTypeSupportsMap[ type ].preview
) {
event.preventDefault();
setSelection( [ item.id ] );
}
} }
>
{ decodeEntities(
item.title?.rendered || item.slug
Expand Down Expand Up @@ -250,18 +261,45 @@ export default function PagePages() {

// TODO: we need to handle properly `data={ data || EMPTY_ARRAY }` for when `isLoading`.
return (
<Page title={ __( 'Pages' ) }>
<DataViews
paginationInfo={ paginationInfo }
fields={ fields }
actions={ actions }
data={ pages || EMPTY_ARRAY }
isLoading={
isLoadingPages || isLoadingStatus || isLoadingAuthors
}
view={ view }
onChangeView={ onChangeView }
/>
</Page>
<>
<Page title={ __( 'Pages' ) }>
<DataViews
paginationInfo={ paginationInfo }
fields={ fields }
actions={ actions }
data={ pages || EMPTY_ARRAY }
isLoading={
isLoadingPages || isLoadingStatus || isLoadingAuthors
}
view={ view }
onChangeView={ onChangeView }
/>
</Page>
{ viewTypeSupportsMap[ view.type ].preview && (
<Page>
<div className="edit-site-page-pages-preview">
{ selection.length === 1 && (
<SideEditor
postId={ selection[ 0 ] }
postType={ postType }
/>
) }
{ selection.length !== 1 && (
<div
style={ {
display: 'flex',
flexDirection: 'column',
justifyContent: 'center',
textAlign: 'center',
height: '100%',
} }
>
<p>{ __( 'Select a page to preview' ) }</p>
</div>
) }
</div>
</Page>
) }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally this is part of the view component. Maybe a slot?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why a slot? Why it's not bundled in the view from the start ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it needs to be rendered into a separate Page component?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree with it being a small outcome

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mcsf I don't know how you see that work? This is the view component area? How do you fit in a preview?

image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than a slot, not sure what solutions you all have in mind?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm not sure now. And Slot might be overkill.

</>
);
}
14 changes: 14 additions & 0 deletions packages/edit-site/src/components/page-pages/side-editor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
/**
* Internal dependencies
*/
import Editor from '../editor';
import { useInitEditedEntity } from '../sync-state-with-url/use-init-edited-entity-from-url';

export default function SideEditor( { postType, postId } ) {
useInitEditedEntity( {
postId,
postType,
} );

return <Editor />;
}
8 changes: 6 additions & 2 deletions packages/edit-site/src/components/sync-state-with-url/use-init-edited-entity-from-url.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,7 @@ import {

const { useLocation } = unlock( routerPrivateApis );

export default function useInitEditedEntityFromURL() {
const { params: { postId, postType } = {} } = useLocation();
export function useInitEditedEntity( { postId, postType } ) {
const { isRequestingSite, homepageId, url } = useSelect( ( select ) => {
const { getSite, getUnstableBase } = select( coreDataStore );
const siteData = getSite();
Expand Down Expand Up @@ -92,3 +91,8 @@ export default function useInitEditedEntityFromURL() {
setNavigationMenu,
] );
}

export default function useInitEditedEntityFromURL() {
const { params = {} } = useLocation();
return useInitEditedEntity( params );
}
Loading