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

[docs] Add signed in state as default on <Account /> docs #3970

Merged
merged 16 commits into from
Aug 30, 2024
Merged

[docs] Add signed in state as default on <Account /> docs #3970

merged 16 commits into from
Aug 30, 2024

Conversation

bharatkashyap
Copy link
Member

@bharatkashyap bharatkashyap commented Aug 20, 2024 •
edited by Janpot
Loading

@bharatkashyap bharatkashyap added the docs Improvements or additions to the documentation label Aug 20, 2024
apedroferreira
apedroferreira requested changes Aug 21, 2024
View reviewed changes
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.

For some reason in https://deploy-preview-3970--mui-toolpad-docs.netlify.app/toolpad/core/react-account/ the demo appears logged out to me at the start. If I click the code editor for example, it changes to a logged-in state.

Also I'm not sure if the preview code shown for the first demo is explanatory enough without being expanded, but not sure about this... I guess it's useful for people to know the format of the session object from there, but as for the signIn function I guess that's covered in another component so it's fine?

@bharatkashyap
Copy link
Member Author

bharatkashyap commented Aug 22, 2024 •
edited
Loading

For some reason in https://deploy-preview-3970--mui-toolpad-docs.netlify.app/toolpad/core/react-account/ the demo appears logged out to me at the start. If I click the code editor for example, it changes to a logged-in state.

You're right, although I can't replicate during local development. Looking into this. Turns out I hadn't run pnpm docs:typescript:formatted

I've added a proposal to detect changes to .tsx files inside docs/data/toolpad/core/* and run pnpm:docs:typescript:formatted if so before running pnpm docs:dev. @Janpot Wdyt?

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 22, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 22, 2024
apedroferreira
apedroferreira approved these changes Aug 23, 2024
View reviewed changes
Copy link
Member

@apedroferreira apedroferreira left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good!

Does the script check for unstaged files only? Maybe that would be better if not but not even sure, anyway I think this is a helpful change, helps not always having to run the docs scripts when working on the docs!
As an alternative, docs:dev could maybe also just watch for live changes in those files and run the script every time they change, but that might be too heavy so then in that case it should probably just generate files for the documentation that changed. But that's just a possible idea for a future improvement!

@bharatkashyap
Copy link
Member Author

Removed the script proposal in favour of using the --watch mode on pnpm docs:typescript:formatted, which is proposed to be better documented and improved by the code-infra team.

@Janpot
Copy link
Member

Janpot commented Aug 29, 2024 •
edited
Loading

Few questions

  • Can we have a demo signed in and one signed out? Either in the same demo or in a different one.
  • Why is there such a huge vertical margin in the demo?
  • For the customization demo, can we show a signed in one next to a signed out one?

@bharatkashyap
Copy link
Member Author

bharatkashyap commented Aug 29, 2024 •
edited
Loading

Few questions

  • Can we have a demo signed in and one signed out? Either in the same demo or in a different one.

I'll update the "States" sub section to have two demos, one titled "Signed In" and one titled "Signed out"

  • Why is there such a huge vertical margin in the demo?

I'm using the bg: gradient property which adds the margins by default, but I can switch to using bg: outlined

  • For the customization demo, can we show a signed in one next to a signed out one?

Do you mean showing the avatar next to the Login button, something like this?

Screenshot 2024-08-29 at 8 40 05 PM

@Janpot
Copy link
Member

Janpot commented Aug 29, 2024

Do you mean showing the avatar next to the Login button, something like this?

yes, it underneath each other. or with a label even

@bharatkashyap
Copy link
Member Author

Do you mean showing the avatar next to the Login button, something like this?

yes, it underneath each other. or with a label even

Screenshot 2024-08-29 at 9 04 30 PM ?

@bharatkashyap bharatkashyap merged commit 46d2f71 into mui:master Aug 30, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apedroferreira apedroferreira apedroferreira approved these changes

Assignees
No one assigned
Labels
docs Improvements or additions to the documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add Account example on the component page in the logged in state
3 participants
@bharatkashyap @Janpot @apedroferreira