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.

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

[feat] Add page structure explorer #2246

Merged
merged 25 commits into from
Jul 22, 2023

Conversation

bharatkashyap
Copy link
Member

@bharatkashyap bharatkashyap commented Jun 30, 2023

pageStructure.mov
  • I've read and followed the contributing guide on how to create great pull requests.
  • I've updated the relevant documentation for any new or updated feature
  • I've linked relevant GitHub issue with "Closes #"
  • I've added a visual demonstration under the form of a screenshot or video

@bharatkashyap bharatkashyap changed the title [feat] [wip] Add page structure explorer [feat] Add page structure explorer Jun 30, 2023
@@ -46,3 +46,27 @@ spec:
value:
$$jsExpression: |
`Invalid error: ${invalidError.error?.message}`
- component: PageRow
Copy link
Member

Choose a reason for hiding this comment

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

Accidentally committed?

@Janpot
Copy link
Member

Janpot commented Jul 3, 2023

Very nice, a few things I noticed while trying it out.

  • Something seems to go wrong with the hover state after renaming:

    Screen.Recording.2023-07-03.at.10.32.34.mov

    Would it be hard to integration test?

  • Do you think it's time we start using TreeView for this UI? I feel like it would create denser UI.

  • Should we add some sort of section title above that says "explorer"?

@@ -167,6 +169,7 @@ export default function NodeHud({
onDuplicate,
isOutlineVisible = false,
isHoverable = true,
isHovered = false,
Copy link
Member

@apedroferreira apedroferreira Jul 3, 2023

Choose a reason for hiding this comment

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

Since the dashed border doesn't really just mean the element is "hovered" anymore we might want to give this prop a different name just to avoid confusion. Maybe isPreviewed? Not sure, also not a super big deal.
I understand this would have to change the wording in a lot of places now.

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 think isHovered captures the state still, since the user is "hovering" over the element albeit inside the structure explorer, so an indirect form of hovering.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I think it's fine to keep it.

Copy link
Member

@apedroferreira apedroferreira left a comment

Choose a reason for hiding this comment

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

Looks awesome and works great! Just noticed a couple of issues:

  • When I drag-and-drop to create a column the whole row that contains it collapses in the panel
  • It's not possible yet to press backspace to delete after selecting a row or column in the explorer, but I feel like it would be a really useful feature

@prakhargupta1
Copy link
Member

  • Do you think it's time we start using TreeView for this UI? I feel like it would create denser UI.
  • Should we add some sort of section title above that says "explorer"?

Looks good! +1 for both of these suggestions:

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 12, 2023
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 12, 2023
@bharatkashyap
Copy link
Member Author

bharatkashyap commented Jul 12, 2023

I've updated to address most of the feedback here:

pageStructure.mov

@apedroferreira I've added an implementation of deleting via the explorer which plays the most nicely with undoing, does this make sense to you or are there any other ways of achieving this?

return draft;
// return normalizePageRowColumnSizes(draft);
},
currentView.kind === 'page'
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we can get rid of these currentView.kind === 'page' as I think these actions can only occur in a page view? But I may have forgotten why I added these checks in the other similar deletes...

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 think it's for the types, in which case I can probably replace these conditionals with
...(currentView as Extract<DomView, { kind: 'page' }>),

(event: React.KeyboardEvent<HTMLUListElement>) => {
// delete selected node if event.key is Backspace
if (event.key === 'Backspace') {
appStateApi.update(
Copy link
Member

@apedroferreira apedroferreira Jul 13, 2023

Choose a reason for hiding this comment

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

Seems like the correct logic, except we do may need to normalize the page row column sizes after deleting as done in the other places...
And now that this logic goes beyond just the RenderOverlay component, we should move all of this (deleteing node + deleting orphaned nodes + normalizing column sizes) to somewhere where we can reuse it more agnostically, not sure if that's easy or not.

Copy link
Member Author

@bharatkashyap bharatkashyap Jul 14, 2023

Choose a reason for hiding this comment

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

And now that this logic goes beyond just the RenderOverlay component, we should move all of this (deleteing node + deleting orphaned nodes + normalizing column sizes) to somewhere where we can reuse it more

I've added some changes to try and accomplish this, have a look

onNodeSelect={handleNodeSelect}
onNodeFocus={handleNodeFocus}
onNodeToggle={handleNodeToggle}
onKeyDown={handleNodeDelete}
Copy link
Member

@apedroferreira apedroferreira Jul 13, 2023

Choose a reason for hiding this comment

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

I would separate onKeyDown to something like handleKeyDown, and there if backspace is pressed call a reusable delete function.
That way for example if we allow deleting in the explorer by clicking a button we just have to use the same function that is already there.

if (event.key === 'Backspace') {
appStateApi.update(
(draft) => {
if (!selectedDomNodeId) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you check for if there is a selected node outside of appStateApi.update? That way we don't add a new entry to the undo stack, I think.

Copy link
Member

@apedroferreira apedroferreira left a comment

Choose a reason for hiding this comment

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

Looks good, just added comments on some minor things but still everything is working great!

@prakhargupta1
Copy link
Member

Just played with it, gives so much clarity about the page hierarchy now. Awesome UX! 🙌

Copy link
Member

@apedroferreira apedroferreira left a comment

Choose a reason for hiding this comment

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

Awesome work Bharat, looks really great! Thanks a lot for addressing these!
I still had some thoughts and possible suggestions but I don't think any of them should be that blocking, it's very minor stuff, so I will approve already.
Maybe a small refactor of deleteNode would be the most important one to me!

@bharatkashyap
Copy link
Member Author

@apedroferreira Resolving the comments related to the common delete function extraction since we agreed on a different approach involving refactoring the existing delete

@apedroferreira
Copy link
Member

@apedroferreira Resolving the comments related to the common delete function extraction since we agreed on a different approach involving refactoring the existing delete

Ok, sounds good!

Copy link
Member

@Janpot Janpot left a comment

Choose a reason for hiding this comment

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

Let's go!

@bharatkashyap bharatkashyap merged commit a701954 into mui:master Jul 22, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Page structure explorer
4 participants