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

[material-ui][Slider] Fix customized iOS demo to use updated official colors #39813

Merged
merged 9 commits into from
Dec 28, 2023

Conversation

Super-Kenil
Copy link
Contributor

@Super-Kenil Super-Kenil commented Nov 9, 2023

New Changes: I have updated iOSlider's design as per apple's docs

Old changes: I removed the unnecessary condition which was returning the same value, regardless if the condition is true or false.

👉 https://deploy-preview-39813--material-ui.netlify.app/material-ui/react-slider/#customization

@mui-bot
Copy link

mui-bot commented Nov 9, 2023

Netlify deploy preview

https://deploy-preview-39813--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against a07f957

@Super-Kenil Super-Kenil changed the title FIX: Removed unnecessary condition [material-ui][docs][data][slider] Fix Removed unnecessary condition Nov 9, 2023
@mj12albert mj12albert added docs Improvements or additions to the documentation component: slider This is the name of the generic UI component, not the React module! labels Nov 9, 2023
@mj12albert
Copy link
Member

Since it's the IOSSlider demo, maybe we should use the latest official colors? The latest guidelines have distinct palettes for different color modes CC @zanivan

@Super-Kenil Super-Kenil changed the title [material-ui][docs][data][slider] Fix Removed unnecessary condition [material-ui][docs][data][slider] Fix IOSSlider now uses updated official colors Nov 9, 2023
@mj12albert mj12albert added the design This is about UI or UX design, please involve a designer label Nov 9, 2023
@danilo-leal danilo-leal changed the title [material-ui][docs][data][slider] Fix IOSSlider now uses updated official colors [material-ui][Slider] Fix customized iOS demo to use updated official colors Nov 9, 2023
@danilo-leal danilo-leal added the package: material-ui Specific to @mui/material label Nov 9, 2023
@@ -34,7 +34,7 @@ const marks = [
];

const IOSSlider = styled(Slider)(({ theme }) => ({
color: theme.palette.mode === 'dark' ? '#3880ff' : '#3880ff',
color: theme.palette.mode === 'dark' ? '#419dfe' : '#0141dd',
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on the Apple Design Figma kit, the current colors are these:

Suggested change
color: theme.palette.mode === 'dark' ? '#419dfe' : '#0141dd',
color: theme.palette.mode === 'dark' ? '#0A84FF' : '#007AFF',

@zanivan
Copy link
Contributor

zanivan commented Nov 10, 2023

Hey @Super-Kenil thanks for the submission!

I wonder if we shouldn't use this chance to update the visual to look more like the current iOS version:

image

@Super-Kenil
Copy link
Contributor Author

Since it's the IOSSlider demo, maybe we should use the latest official colors? The latest guidelines have distinct palettes for different color modes CC @zanivan

I wonder if we shouldn't use this chance to update the visual to look more like the

I think I can make it look the current version, just like you mentioned. I will update it and commit.

But I am still facing issues related to passing one of the checks of Argos
Because I am not sure what I am supposed to do about it.
I have attached images below, please let me know @zanivan or @mj12albert

@mj12albert
Copy link
Member

I think I can make it look the current version, just like you mentioned. I will update it and commit.

👍👍👍

@Super-Kenil Don't worry about the argos check! Sometimes it can be very flaky, it can literally be the very last thing we deal with for this PR 😬

@Super-Kenil
Copy link
Contributor Author

I know this is not the right place @mj12albert @zanivan , but I am trying to contribute to the docs, and I am facing issue when I started the dev server on my machine, I tried to fix it myself, but I surely am missing something. This is my first time contributing anywhere, so any help would be appreciated. Checkout the attached image, it shows the error I am getting.

image

@mj12albert
Copy link
Member

it shows the error I am getting.

@Super-Kenil Hey, could you push your latest commits to this branch, even if there are errors? I can check out your branch and have a look

@Super-Kenil
Copy link
Contributor Author

@mj12albert , I did it, you can have a look there. And yes, the project didn't start even when I had not made any changes, so there must be something I am missing as this is my first ever contribution

@mj12albert
Copy link
Member

mj12albert commented Dec 4, 2023

@Super-Kenil You've done yarn install and then yarn start (using yarn v1, not latest) from this guide right?

I don't see anything wrong with your branch btw, it runs fine for me 🤔

@Super-Kenil
Copy link
Contributor Author

@mj12albert Then I will try again with latest version of yarn today, and make changes, thanks for letting me know about yarn thing.

@ZeeshanTamboli
Copy link
Member

@Super-Kenil Any updates on this?

@Super-Kenil
Copy link
Contributor Author

@ZeeshanTamboli , I have been busy lately with my new nextjs project, which I will be finishing up in soon, So. I will be updating the design very soon

@Super-Kenil
Copy link
Contributor Author

The iOS slider has been updated to latest design just like @zanivan mentioned

@Super-Kenil
Copy link
Contributor Author

Super-Kenil commented Dec 23, 2023

@mj12albert can you tell me, exactly which yarn version are you using, cause I have been facing same errors even after updating to yarn latest like you mentioned with the following guide .

I don't get those errors when I visit any components page, I am able to visit all pages other pages, where it doesn't contain examples.

If possible then guide me as well, although the design has be done, and the PR is ready to get merged, but I can help in future too if at least I will be able to install the dependencies.

Signed-off-by: Kenil Sudani <kenilsudani.blog@gmail.com>
Copy link
Contributor

@danilo-leal danilo-leal left a comment

Choose a reason for hiding this comment

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

Looks great, appreciate it! (Though I might fine-tune this a bit further on an upcoming Slider revision page). 😬 But we should get this one out of the door already!

@danilo-leal danilo-leal merged commit 0fcc3e0 into mui:master Dec 28, 2023
19 checks passed
mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Jan 9, 2024
… colors (mui#39813)

Signed-off-by: Kenil Sudani <kenilsudani.blog@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: slider This is the name of the generic UI component, not the React module! design This is about UI or UX design, please involve a designer docs Improvements or additions to the documentation package: material-ui Specific to @mui/material
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants