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

Allow <Title> to be non configurable #9836

Merged
merged 10 commits into from
Jun 28, 2024

Conversation

markyao6275
Copy link
Contributor

@markyao6275 markyao6275 commented May 8, 2024

This addresses #8776

Testing:

Added storybook entry

Screenshot 2024-05-08 at 9 51 04 AM

@markyao6275
Copy link
Contributor Author

@fzaninotto @adguernier I believe this is ready for review again. Thanks!

@adguernier
Copy link
Contributor

adguernier commented May 22, 2024

Reviewed. Thanks for your work @markyao6275 👍

@markyao6275
Copy link
Contributor Author

Thanks for the comments @adguernier! Should be ready for another review :)

@adguernier
Copy link
Contributor

Aside from the last two comments, LGTM. Thanks for your work 💪

@djhi djhi self-requested a review May 23, 2024 09:13
Copy link
Collaborator

@djhi djhi left a comment

Choose a reason for hiding this comment

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

Thanks! I think the default preferenceKey in Title should be the same as in PageTitleConfigurable and you should check if preferenceKey is strictly equal to false.

@markyao6275
Copy link
Contributor Author

@djhi you mean I should check if preferenceKey strictly equal to undefined right?

Anyway I tried to address your feedback in the most recent commit so please let me know if that's what you had in mind.

@djhi
Copy link
Collaborator

djhi commented May 24, 2024

@djhi you mean I should check if preferenceKey strictly equal to undefined right?

I mean that to have it work like it did before, we should have a default preferenceKey set and users have to explicitly disable it by setting it to false. Hence, you should check if it is strictly equal to false to render the non configurable version. This is coherent with other part of react-admin.

@markyao6275
Copy link
Contributor Author

@djhi just acted on your feedback

For

I mean that to have it work like it did before, we should have a default preferenceKey

I just passed through the preferenceKey from <Title /> to <PageTitleConfigurable /> so that I don't have to specify the default ${pathname}.title in more than one place.

Let me know if there's anything else you'd like me to change.

@markyao6275
Copy link
Contributor Author

@djhi friendly bump here

@markyao6275 markyao6275 requested a review from djhi June 13, 2024 02:24
@markyao6275
Copy link
Contributor Author

@djhi addressed, thanks!

@markyao6275
Copy link
Contributor Author

@djhi friendly bump here

Copy link
Contributor

@slax57 slax57 left a comment

Choose a reason for hiding this comment

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

Other than that, it works fine, thanks! 🙂

@markyao6275
Copy link
Contributor Author

@slax57 addressed your comments, thanks!

@slax57 slax57 added this to the 5.1.0 milestone Jun 28, 2024
@slax57 slax57 merged commit 3a9de0c into marmelab:next Jun 28, 2024
13 checks passed
@slax57 slax57 changed the title Allow <Title /> to be nonConfigurable Allow <Title> to be non configurable Jun 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants