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

Feature/constrain parent pages #21

Merged
merged 8 commits into from
Apr 10, 2024
Merged

Conversation

ThomasBroch
Copy link
Contributor

This PR expands the tree config with an option to define, for each document type, which types of documents they can have as parents.

This feature has been implemented in three areas:

  • The generation of the parent field defenition. There, the references to also take this (optional) setting into account.
  • The new-page + button menu in the page tree layout.
  • If the above two fail, there is also a validator that fetches the type of the selected parent document and validates it against the allowed parent types, of those are available.

@ThomasBroch ThomasBroch marked this pull request as ready for review February 23, 2024 12:03
@ThomasBroch ThomasBroch requested a review from djohalo2 February 23, 2024 12:04
@tim-vyg
Copy link

tim-vyg commented Mar 14, 2024

Love this feature!

Copy link
Member

@tstikvoort tstikvoort left a comment

Choose a reason for hiding this comment

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

Great! There a couple of typo's.

The only thing I am missing is an empty state, when non of the pages has marked the page type in their allowedParents.

E.g. in the following configuration. You cannot create a child content page of a content page:

  allowedParents: {
    'contentPage': ['homePage'],
  }

This is a valid case, so we should support this in the UI as well

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/validators/parent-validator.ts Outdated Show resolved Hide resolved
src/validators/parent-validator.ts Outdated Show resolved Hide resolved
src/validators/parent-validator.ts Outdated Show resolved Hide resolved
src/types.ts Outdated Show resolved Hide resolved
@ThomasBroch
Copy link
Contributor Author

@tstikvoort Good catch! In the empty state, it now disables the button and shows this tooltip:
Screenshot 2024-04-09 at 09 58 25

Copy link
Member

@djohalo2 djohalo2 left a comment

Choose a reason for hiding this comment

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

Very nice! I think we can slightly improve the empty state text and then this can be merged 👍

const tooltipContent = isAddPageButtonDisabled ? (
<Box padding={1}>
<Text muted size={1}>
The allowed parents configuration prevents this document type from being a parent to any other document.
Copy link
Member

@djohalo2 djohalo2 Apr 10, 2024

Choose a reason for hiding this comment

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

I think this text is just a little bit too technical for the average content editor. We can't assume the use case of why it is configured to not contain child pages, so my suggestion is to just keep it simple. Maybe something like This page cannot contain any child pages. or something similar?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is one that I had been debating myself as well for longer than I would like to admit.. As, on the one hand, I don't want to get too technical. But on the other hand, I want to make it informative enough as well that people know what's up. Furthermore, I tried to avoid the word "child", as this empty state is a result of an allowed parent config. How informative do you think the tooltip will need to be?

@djohalo2 djohalo2 merged commit 6fd867b into main Apr 10, 2024
@djohalo2 djohalo2 deleted the feature/constrain-parent-pages branch April 10, 2024 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants