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] Remove empty tags on the TransferList demos #33127

Merged
merged 4 commits into from Dec 21, 2022

Conversation

ekusiadadus
Copy link
Contributor

@ekusiadadus ekusiadadus commented Jun 13, 2022

Why

I found a JSX closing tag error in docs about TransferList and fixed it.

@mui-bot
Copy link

mui-bot commented Jun 13, 2022

No bundle size changes

Generated by 🚫 dangerJS against e9d4a8f

@mnajdova
Copy link
Member

I can see the same in the SelectAllTrasferList.jsx|tsx. Would you mine updating those too?

@mnajdova mnajdova added docs Improvements or additions to the documentation component: transfer list This is the name of the generic UI component, not the React module! bug 🐛 Something doesn't work labels Jun 22, 2022
@ekusiadadus
Copy link
Contributor Author

@mnajdova
Thanks for your reviewing.

I have also fixed SelectAllTransferList.

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

This is not a broken closing tag, it is a self closing tag. Removing it broke the demo, for example try the basic demo - https://deploy-preview-33127--material-ui.netlify.app/material-ui/react-transfer-list/#basic-transfer-list. I am closing it, sorry for the initial misleading comment.

@mnajdova mnajdova closed this Jul 12, 2022
@ekusiadadus
Copy link
Contributor Author

@mnajdova

I checked it again and I still think it contains unnecessary tags. <- it's not broken, but unnecessary.

The demo did not appear to be broken either.

Could you to check it again?

@mnajdova
Copy link
Member

@mnajdova

I checked it again and I still think it contains unnecessary tags. <- it's not broken, but unnecessary.

The demo did not appear to be broken either.

Could you to check it again?

The demo is broken in https://deploy-preview-33127--material-ui.netlify.app/material-ui/react-transfer-list/#basic-transfer-list, for example try to move the first list item to the right.

@ekusiadadus
Copy link
Contributor Author

ekusiadadus commented Nov 10, 2022

@mnajdova

I checked it once again, and recorded it...
I didn't think it was broken.

A self closing tag on this line is not necessary...

Transfer.list.React.component.-.Material.UI.-.Google.Chrome.2022-11-10.23-25-26.mp4

@mnajdova
Copy link
Member

@ekusiadadus I was refering to the example in the PR: https://deploy-preview-33127--material-ui.netlify.app/material-ui/react-transfer-list/#basic-transfer-list I can't see if the code on your side is changed, but it is clearly broken on the PR preview:

transfer-list-bug

@ekusiadadus
Copy link
Contributor Author

@mnajdova

Thanks for your review and sample movie.
But, still I'm not sure it's broken....

You clicked 'Transfer All Item' button, is it right?
Then the code works correct.

If you want to transfer only selected items, please click the button below.

I've checked my code and preview again, and still I think this code works correct.

@mnajdova
Copy link
Member

You clicked 'Transfer All Item' button, is it right?
Then the code works correct.

🤦‍♀️ you are right, I kept pressing the Trasfer all button for some reason.

@mnajdova mnajdova reopened this Dec 14, 2022
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, it was my bad :)

@mnajdova mnajdova changed the title fix: JSX closing tag error on TransferList docs [docs] Remove empty tags on the TransferList demos Dec 14, 2022
@mnajdova mnajdova merged commit e0e0d69 into mui:master Dec 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: transfer list This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants