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

[docs] Revise Joy UI "Input" page #35970

Merged
merged 6 commits into from
Feb 8, 2023
Merged

[docs] Revise Joy UI "Input" page #35970

merged 6 commits into from
Feb 8, 2023

Conversation

LadyBluenotes
Copy link
Contributor

@LadyBluenotes LadyBluenotes commented Jan 28, 2023

Hi @samuelsycamore,

I did a few edits on this one to make it (what I assume) is similar to what you had mentioned previously in the Typography doc. It looks like this one was already pretty much good to go - whoever originally wrote it did a great job at following your template!

I added a few things that I think may help with clarity for people reading the document. I did have one question, however. When looking at the component, I'm not sure if there is a way to determine what type of input is being used. For example, if you wanted to use this component in a form and the user was to input a password, would the developer have to pass a prop into the inner HTML element to specify the type? I was thinking about adding something to clarify this point, just because it was a question I had myself.

Let me know if there's any changes / edits I need to make!

@mui-bot
Copy link

mui-bot commented Jan 28, 2023

Messages
📖 Netlify deploy preview: https://deploy-preview-35970--material-ui.netlify.app/

No bundle size changes

Generated by 🚫 dangerJS against 4cf8fd0

@oliviertassinari oliviertassinari added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI docs Improvements or additions to the documentation component: text field This is the name of the generic UI component, not the React module! package: joy-ui Specific to @mui/joy labels Jan 28, 2023
@samuelsycamore
Copy link
Member

Thanks for taking on another one @LadyBluenotes ! It looks like many tests are failing. Whenever you submit a new PR, make sure that it runs and builds locally first. This will help prevent failing tests and broken deploy previews.

In this case, it looks like we have a few issues:

  • the <p class="description"> has a missing bracket, causing the page to break
  • the standard "cleanup" checks are failing. Based on the tests above, you need to run:
    • yarn prettier
    • yarn markdownlint
    • yarn docs:typescript:formatted
      • I make it habit to run all three pretty much every time I commit changes

Once you get this bad boy building and passing tests, then I'll circle back with a line-by-line review. 👍

@LadyBluenotes
Copy link
Contributor Author

🤦 I was wondering why the checks were failing. Thank you for pointing them out!

@samuelsycamore
Copy link
Member

It worked! When in doubt, run the command twice I guess. 🤷‍♂️

@LadyBluenotes
Copy link
Contributor Author

Woohoo! I'll make sure to run it at the end before pushing next time. Hopefully it was just this once it decided to be fussy :')

@LadyBluenotes
Copy link
Contributor Author

Is there anything I need to update/edit for this page?

@samuelsycamore
Copy link
Member

Thanks @LadyBluenotes ! I made some copy edits throughout, mostly all stuff that's easier for me to just do myself than to try to explain and then make you do it. You can click on my copyediting commit up above to see what I changed. Mostly tweaking words to conform to standard copy we've established on other pages. In a few instances I reworded some kinda awkward passive sentences to make them active.

Let's wait for a final review from @danilo-leal or @siriwatknp but I'd say this is probably good to go.

@samuelsycamore samuelsycamore removed the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Feb 7, 2023
Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

🚀 A lot better!

@samuelsycamore samuelsycamore merged commit 6f758f3 into mui:master Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: text field This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation package: joy-ui Specific to @mui/joy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants