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][base-ui] Add Tailwind & plain CSS demos for Autocomplete page #38157

Merged
merged 15 commits into from
Aug 16, 2023

Conversation

mj12albert
Copy link
Member

@mj12albert mj12albert commented Jul 25, 2023

@mj12albert mj12albert added docs Improvements or additions to the documentation component: autocomplete This is the name of the generic UI component, not the React module! package: base-ui Specific to @mui/base labels Jul 25, 2023
@mj12albert mj12albert self-assigned this Jul 25, 2023
@mui-bot
Copy link

mui-bot commented Jul 25, 2023

Netlify deploy preview

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against b358d6c

@mj12albert
Copy link
Member Author

@zanivan I've adjusted the proportions of the Autocomplete intro to match #37836 and #37750, would you mind having a quick look before I go on to make the plain CSS one and the rest of the demos on the page ~

Preview: https://deploy-preview-38157--material-ui.netlify.app/base-ui/react-autocomplete/#introduction

@zanivan
Copy link
Contributor

zanivan commented Jul 26, 2023

Hey, that's great! I've just added some tweaks to box-shadow, but the proportions look good!

Just one quick question, will you create the plain CSS and Tailwind demos for all the demos on the page? Otherwise, if you're only doing one, perhaps would be better to start with the Basics demo, to keep it consistent with the other components

@mj12albert mj12albert force-pushed the base/tailwind-autocomplete branch 2 times, most recently from 3961e65 to f42b17b Compare July 28, 2023 05:19
@mj12albert
Copy link
Member Author

@zanivan Planning to do them all as the design is more or less identical ~ do you wanna exclude the introduction one for consistency though 😅

@mj12albert mj12albert force-pushed the base/tailwind-autocomplete branch 2 times, most recently from 374324d to 6e957b0 Compare July 28, 2023 11:53
@zanivan
Copy link
Contributor

zanivan commented Jul 28, 2023

No worries, if you're adapting all the demos it'll still be consistent—and also very helpful ✨

@mj12albert mj12albert force-pushed the base/tailwind-autocomplete branch 4 times, most recently from b9705bf to 9084aa8 Compare July 31, 2023 05:35
@mj12albert mj12albert marked this pull request as ready for review July 31, 2023 05:42
@mj12albert mj12albert requested review from zanivan and a team July 31, 2023 06:07
@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 31, 2023

The toggles on the introduction demo looks great.

However, the toggle on the other is a no-no as long as the styles/source are duplicated between each demo.
We don't have the resources to maintain this at a high level of execution. We have all the demos of https://mui.com/material-ui/react-autocomplete/ to migrate to Base UI (no need to have most of them duplicated in Material UI IMHO).

Instead, I would propose we follow #37222 (comment), which will involve some docs-infra work to pull-off.

@oliviertassinari oliviertassinari changed the title [docs][base] Add Tailwind & plain CSS demos for Autocomplete page [docs][base-ui] Add Tailwind & plain CSS demos for Autocomplete page Jul 31, 2023
@mj12albert
Copy link
Member Author

the toggle on the other is a no-no as long as the styles/source are duplicated between each demo.

@oliviertassinari do you mean to only add the Tailwind/plain CSS demos for the intro then and keep all the rest as system-only ?

Here it is the most complete one, as it includes use of Popper while the "Basics" one does not CC @zanivan

@oliviertassinari
Copy link
Member

@mj12albert I mean that the heavy lifting of the component style, I think it should only be done once. I would propose that the demos that come after the introductory one either import the introduction component or be as simple as possible to explain the concept.

@zanivan
Copy link
Contributor

zanivan commented Aug 2, 2023

Just to clarify, @oliviertassinari are you suggesting that we add the Tailwind and Plain CSS style selector only to the first demo?
If that's the idea, I wonder if it wouldn't harm the easiness of copying and pasting code from demos, regardless of the chosen style engine. I agree, though, that having the styles on all demos could harm the scalability, because it'd be very hard to maintain all these files.

Or, do you mean that we should have the style selector in all demos, but the demos from different styles shouldn't be visually different? If so, I agree with this and I resonate with your comment on #37222. We should definitely start using Joy UI design language on Base UI demos, but, as you said, it would be exhaustive to revamp all Base UI demos at once.

I propose we keep this discussion centralized on the Notion page created by @mnajdova to discussing this matter, at least up until we have a clear approach. In the meantime, we can continue adding styles only to the Basics and Introduction demos and also add to #37222 to keep track of these.

import ClearIcon from '@mui/icons-material/Clear';
import clsx from 'clsx';

const CustomAutocomplete = React.forwardRef(function CustomAutocomplete(props, ref) {
Copy link
Member

@oliviertassinari oliviertassinari Aug 2, 2023

Choose a reason for hiding this comment

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

What if we were removing all of these Custom, Styled, or prefixes to keep it raw?

Suggested change
const CustomAutocomplete = React.forwardRef(function CustomAutocomplete(props, ref) {
const Autocomplete = React.forwardRef(function Autocomplete(props, ref) {

#38256 (comment)

It would be one step closer to how developer would export and use these components in their codebase.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense, I have renamed the components in the demos c62eb62

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 2, 2023

Just to clarify, @oliviertassinari are you suggesting that we add the Tailwind and Plain CSS style selector only to the first demo?

@zanivan I think it's OK to have the switch on all the demos, however:

  1. The Introduction demo feels like the only one that truly matters, if we only have multiple styles and it's greatly done, I think it will get us 80% there.
  2. For the others, unless it's automated, it feels like a negative ROI. Or at minimum, I would expect the path to user feedback to be easy enough for them to start to complain so we could start without.

Why? because 1. is what you will copy and paste in your code 2. is mostly what you will read to understand.

but the demos from different styles shouldn't be visually different?

No strong preference for this. I like the path of having the exact same style to keep it simple. This would reduce the combinatory of color contrasts we need to check to make sure it feels great. So increase our chance to provide a consistent great UX.

In the meantime, we can continue adding styles only to the Basics and Introduction demos and also add to #37222 to keep track of these.

I would even say Introduction is enough. On all the pages of Base UI, the Basic section feels like one we could remove tomorrow while improving the DX because it feels like a systematic duplicate of the Introduction section.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 3, 2023
@mj12albert mj12albert force-pushed the base/tailwind-autocomplete branch from c62eb62 to 62d128f Compare August 7, 2023 11:54
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 7, 2023
@mj12albert mj12albert force-pushed the base/tailwind-autocomplete branch from 62d128f to b358d6c Compare August 7, 2023 12:04
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.

LGTM

@mnajdova
Copy link
Member

I scoped #37222 to only the Introduction demos.

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Looks good 👍

  1. In the introduction section, It might be great to have the source of the component in a separate source tab. In the case of the autocomplete const top100Films = [ takes a lot of space. In the other pages, it would help to copy the source of the component.
  2. @zanivan The style of the autocomplete connects back to this feedback on Joy UI: https://www.notion.so/mui-org/Olivier-design-review-on-Joy-Design-3ada9a7bcfa44b9fab1fe5032dfb33bb?pvs=4#40c4519ac5bf4e8d9f721f2d51377f9c. I think we should pick a side 😁
Screenshot 2023-08-15 at 15 37 42
  1. It feels like we should invest in style conversion automation. There is so much code to convert to Tailwind CSS and CSS, I fail to see how doing it manually is more efficient. If we could turn [docs][base-ui] Add plain CSS, MUI System, Tailwind CSS and non-styled option for the Introduction demos #37222 into a good to take to delegate to the community, we could maybe get away without automation for some time, by delaying it, but long term I don't see how.

@mnajdova
Copy link
Member

In the introduction section, It might be great to have the source of the component in a separate source tab. In the case of the autocomplete const top100Films = [ takes a lot of space. In the other pages, it would help to copy the source of the component.

There are some disadvantages to this, especially for short demos:

  • I can't just copy and paste it locally anymore, I will have to create few files and then copy each file
  • have to switch between tabs if I want to see the source of the component vs how it is used

I like the idea, I just want to emphasize that it is not a silver bullet

It feels like we should invest in style conversion automation. There is so much code to convert to Tailwind CSS and CSS, I fail to see how doing it manually is more efficient. If we could turn #37222 into a good to take to delegate to the community, we could maybe get away without automation for some time, by delaying it, but long term I don't see how.

We could use tools like this for the conversion of the styles - https://tailwind-converter.netlify.app/. I am not sure if the automation will scale, for e.g. we use different methodology for creating the components, styled vs using slotProps. It would mean that we should match the class name selectors to slot names. More over, the tokens would kind of need to be matched manually (only once so that's fine). If we want to try it, we could do it with the select component as a test case, but honestly I feel like we are going a lot of time on it, and there will always be cases that won't be supported :) Maybe I am wrong, but that is my takeaway.

@zanivan
Copy link
Contributor

zanivan commented Aug 15, 2023

@zanivan The style of the autocomplete connects back to this feedback on Joy UI: https://www.notion.so/mui-org/Olivier-design-review-on-Joy-Design-3ada9a7bcfa44b9fab1fe5032dfb33bb?pvs=4#40c4519ac5bf4e8d9f721f2d51377f9c. I think we should pick a side 😁

It indeed looks better! @siriwatknp How hard would it be to add a margin on the Joy UI select list, and a border radius to the items? If you're ok with this, I think we can open a PR to address it.

@mj12albert
Copy link
Member Author

Thanks everyone for the review, I will merge this first!

@mj12albert mj12albert merged commit 85c3c77 into mui:master Aug 16, 2023
@mj12albert mj12albert deleted the base/tailwind-autocomplete branch August 28, 2023 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: autocomplete This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation package: base-ui Specific to @mui/base
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants