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

Adding custom CSS styles #4420

Closed
vfonic opened this issue Aug 23, 2021 · 28 comments
Closed

Adding custom CSS styles #4420

vfonic opened this issue Aug 23, 2021 · 28 comments

Comments

@vfonic
Copy link

vfonic commented Aug 23, 2021

Feature request summary

I want to make some CSS changes to the way the Polaris components are rendered in my Shopify app.
I see there's no way to pass style or className prop to any of the Polaris components.

Rationale

I believe there should be a way to easily add custom CSS styles to Polaris components.
Otherwise, I'd have to add a non-Polaris React component, style it and only inside of it add a Polaris component.

This is not ideal and could result in additional issues because I'm adding another component and not really styling the same Polaris component, but its wrapper component.

In CSS-only version of Polaris, this is as simple as adding another CSS class, but React Polaris components don't support className prop.

Any ideas how to achieve this?

@vanduc1102
Copy link

vanduc1102 commented Oct 20, 2021

I am having the same issue
In my case, I could not customize the Modal Header close button (X) ,
because the modal is rendered in a portal, I had to hide the modal close button, and created my own button

@jbalsas
Copy link
Member

jbalsas commented Dec 14, 2021

I was searching for related issues and this is the first one that showed up.

Full disclosure, this was already discussed and rejected back in 2017 in Allow to add css classes to any components

I think one of the main takes can be found in this comment:

[...] the restrictions against extension are by design: this is not meant to be a general-purpose library, so most of the visual styling should not be changed, and we want to be explicit about the variations we endorse by making these full-fledged variations (by adding dedicated props).

And the decision was summarized in this other comment:

We won't be adding the ability to set any custom class names/ styles on any of the Polaris components. It goes against how we think about components by making the markup part of the component's API, and means that we are much more likely to break things in consuming apps because we don't know what random variations people have made to our components. It also greatly complicates the UI, as we'd probably need to expose the ability to add class names to almost any node in the tree output by a component.

@jbalsas
Copy link
Member

jbalsas commented Dec 14, 2021

@lemonmade, I don't mean to drag you into this conversation again, but this recently came up in the context of some changes we want to do to Shopify's admin and I'd like to double check if we still stand by this decision.

TL;DR: Is this something that Polaris would be open to explore again?


Some context:

  • We wanted to use the WandMajor icon in a smaller context (would require a minor icon).
  • To avoid creating a new icon, I added the following CSS
.Icon {
  [class*='Polaris-Icon'] {
    // We're using WandMajor, scale it to make it 16x16
    transform: scale(0.8);
  }
}

While it's true we can always (more or less) find alternatives, a quick search for \[class[^*?]=’Polaris in our codebase returns 120 results - 42 files and I'd venture there are other ways in which variations of Polaris components are being created.


It's obvious to me that this is by design, so it's fine if we want to stand by it. I wanted to bring the attention to the idea of "enforcing all possible needed variations" as part of the framework.

Trying to reach 100% coverage of use cases could produce some issues like:

  • A stalemate that blocks both the Polaris team and FED teams working in admin, unable to move without the other one in sync
  • A bloated design system and code base with plenty one-offs and hardly used variants
  • Escape hatches like \[class[^*?]=’Polaris to try to GSD

Trying to aim for an 80/20 rule could provide some additional leverage. For example, providing className only to the boundingBox of components could provide a sanctioned and trackable mechanism that would more safe-ish-ly cover the needs of that 20% that shouldn't necessarily make it into Polaris.

What falls in the 80-20 ratio is of course something that changes over time. Things that were once part of the exceptions can become mainstream enough so that the pattern is worth reintegrating in the library and things that were common may fall out of fashion and deprecated/removed. We could be monitoring these patterns and react accordingly to keep a healthy relationship between library and consumers.

@vfonic
Copy link
Author

vfonic commented Dec 14, 2021

Thank you, @jbalsas! Great points!

As you pointed out, it will be impossible to cover all of the edge cases.

In my layouts, 80% of the Polaris components are left untouched. However, there's that 20% that simply cannot be done using Polaris. Polaris is a great library, but let's face it, it's far from complete, and far from covering all edge cases. It's simply impossible.

I used to work in Microsoft. We recreated HTML5 spec in C# and tried to force everyone to build webpages by using those C# classes/interfaces/methods. We were trying to cover all possible edge cases (note that this is a complete spec, not UI library, like Polaris, which is infinitely more complex and limitless) such as: img tag must have alt, a tag with target="_blank" must have proper accessibility attributes, etc. Of course, the spec was changing over time and we had to catch up and, sometimes people had to go around the existing C# classes, so we ended up with many teams having their own HTML5 spec extensions (on top of ours). It. Was. A. Disaster. And also very difficult to use.

Please don't repeat our mistakes.
Yes, adding className (to bounding component) greatly increases the API. I don't see how it increases the chance of breaking UIs due to custom overrides any more than any of the workarounds that developers already do. No, it doesn't work to force developers to "just use Polaris without custom styles", as you've seen even in your own codebase.

Developers should know to check if there's anything broken in their UI, after they upgrade Polaris, especially if they have custom styles, but even if they don't. With great power comes great responsibility. Shopify monolith is written in Ruby programming language. I love Ruby and I love the almost-limitless freedom it gives me to write my application's code. It's a great developer experience.

Adding className prop to Polaris components only improves developer experience.

Every single full-page layout in my codebase has this import: import './style_overrides.css';. This really defeats the purpose and it's really difficult to target one given element, so I have to do these kinds of acrobatics:

/* wow, three levels > * > */
.Polaris-Tabs__Panel > * > * {
  margin-top: 3.6rem !important;
}

Maybe there are other libraries with this decision/limitation, but from the top of my head, I cannot remember a single one... :/

@alex-page
Copy link
Member

Hey @vfonic we are exploring flexible layout components to allow you to build that 20% that you are asking for. We are also trying to decide the best way to allow developers to break the rules and extend Polaris. This is a while away but I am going to share this issue with our team.

For now I will close this but appreciate the idea and feedback.

@vfonic
Copy link
Author

vfonic commented May 20, 2022

The fix is extremely easy: allow passing style and/or className props to Polaris components. I don't see the need for having extended discussions about how to achieve this. The functionality is already there and in-a-way you removed it.

@alex-page
Copy link
Member

@vfonic yes that would be an easy fix.

This approach makes our ability to ship changes confidently slower as lots of folks will use this functionality which will cause conflicts, drop confidence from teams using Polaris and make it harder for teams to stay on the latest version. I hear your needs but I am not confident in your suggested approach. I'll try my best to keep this thread updated as we explore a scalable solution for customising CSS.

@vfonic
Copy link
Author

vfonic commented Jun 2, 2022

I understand your concerns, but allow me to respectfully disagree. What are you trying to build with Polaris library? A "complete", full set of UI components for every possible use-case for a Shopify-related UI?

That's a commendable goal. But it's more like a mission/vision than a realistic achievable goal.

Why don't you "take inspiration" for existing UI libraries?

Let's go over a few of the most popular UI libraries:

  1. MUI: https://mui.com/material-ui/customization/how-to-customize/
  • "The sx prop is the best option for adding style overrides to a single instance of a component in most cases."
  • "If you want to override a component's styles using custom classes, you can use the className prop, available on each component."
  1. React-Bootstrap: https://react-bootstrap.github.io/
  • Allows both className and style props (as well as as prop for rendering a completely different component)
  1. Ant Design: https://ant.design/docs/react/getting-started
  • Allows both className and style props
  1. Reactstrap: https://reactstrap.github.io/?path=/story/home-installation--page
  • Allows both className and style props
  1. Semantic UI React: https://react.semantic-ui.com/
  • Allows both className and style props (as well as as prop for rendering a completely different component)
  1. Chakra UI: https://chakra-ui.com/
  • Allows as prop for rendering a completely different component
  • "Style props are a way to alter the style of a component by simply passing props to it. It helps to save time by providing helpful shorthand ways to style components." - it allows any possible style
  1. Theme UI: https://theme-ui.com/
  • Allows sx prop (any styles)
  1. Rebass: https://rebassjs.org/
  • Allows sx prop as well as className prop
  1. Blueprint: https://blueprintjs.com/
  • Allows className prop

I understand these are much more general purpose than Polaris. I'm just trying to find another example of the limitations you're trying to impose. Android UI system? No. iOS UI system? Not even them.
What I'm trying to say is: Polaris is a great, extremely useful component library. Thank you for that! 🙏
Let the UI developers do what they do the best: use the full potential of the underlying UI language. That's when you'll get the best results. Maybe you'll also get some worse-looking results than if you don't allow the full use of (CSS) language.

This approach makes our ability to ship changes confidently slower

That's what automated tests are for. And semantic versioning. And a great CHANGELOG (please click on this link - why am I supposed to know that the CHANGELOG is in polaris-react directory?)

lots of folks will use this functionality which will cause conflicts, drop confidence from teams using Polaris and make it harder for teams to stay on the latest version.

You're absolutely right, it will be harder to do upgrades the more customizations developers make.

How did all of the above-mentioned libraries achieve their span of flexibility while also allowing for relatively easy package upgrades? I don't know. But you could look into that.
Actually, these limitations that you are imposing are dropping my confidence for using Polaris. I was thinking of forking the repo and adding support for style and className props to all components. If I then announced that I'm planning to keep updating my Polaris fork "Polaris++", would that solve the problem? It would create unnecessary mess. (I'm not planning to do this, don't take this as a threat, I'm just speaking hypothetically as similar things have happened to other open-source projects (Elastic search, Mapbox to name a few)

I hear your needs but I am not confident in your suggested approach.

Maybe you know a few examples of some other companies/libraries doing the same, but I looked and couldn't find any other example where overriding styles was not possible. I'm quite confident that if "eveeryone else is", then it's probably you, not everyone else.

I'll try my best to keep this thread updated as we explore a scalable solution for customising CSS.

I'd suggest you keep this issue open. It hasn't been resolved and you're saying you'll provide updates to it. It would be much easier for people to find it and stay up-to-date with your updates.

Sorry for the long rant. For me this is very straightforward. I'm failing to see any benefits of not allowing style and className props.

@vfonic
Copy link
Author

vfonic commented Aug 15, 2022

Hey @alex-page! Are there any updates on this issue?

It's been three months since you said:

we are exploring flexible layout components to allow you to build that 20% that you are asking for.

Let me know if I missed it, but I don't see the way to achieve what I need in Polaris just yet. :/

Again, I'd ask you to reopen this issue, as I believe it hasn't been solved.

I see there are 6 thumbs up on my comments and zero thumbs up on your comments. What do you think about posting a question/poll on Discord/Slack to see what your users (aka app developers) think?

I've hit this issue yet again today. I'm trying to write Playwright automation tests for my app and it's a real pain to target Polaris elements in tests.

Thanks!

@Koala-afurubayashi
Copy link

Hi!!
I want you to pass even just the className to Props
I don't think it has anything to do with version upgrades.
Why do minor css changes have to find their way inside Polaris?

I support vsonic.
we should bring back the closed ISSUEs and present a concrete method for the ideal of 20%.

@kazukinagata
Copy link

Totally agree with vfonic.

@vfonic
Copy link
Author

vfonic commented Sep 20, 2022

Hey @alex-page can you please reopen this issue until it is resolved in some way?

Seems like, even though it's closed, people still find it and participate in it as they, just like me, find it important.

@trongithust
Copy link

I think this issue should be reopen.

@CalebVDang
Copy link

This is an important thread. There are minor tweaks that I want to make without reworking the type of component I'm using.

@adamcharvat
Copy link

It is now 2024 and this is still an issue with Polaris...

@vfonic
Copy link
Author

vfonic commented Jan 31, 2024

@alex-page would be great to have this issue reopened...or at least acknowledge in some way that you're still listening and care about your users (developers)

@Andrey-Pavlov
Copy link

Why is it not allowed to pass className or styles? At least allow us to pass these props through to the first container. The current state of component system design is a bad joke.

@alex-page
Copy link
Member

alex-page commented Mar 1, 2024

Woah woah woah. We care about our developers. You don't need to say things like that to get my attention.

We have zero plans right now to support adding custom css, class names or styles to components. As I said above keeping the admins design as easy to update means we cannot offer escape hatches on our components. If we make a button more "tactile" with a bevel and a background color we need to be sure we can ship this at scale. We want to make it as easy as possible for developers to keep up with the latest design decisions.

We do have two options for developers. These are not hooks into existing components but they let you build custom UI and keep aligned to the system.

  1. Use our tokens. Build whatever you want with our tokens. Make sure to use the right semantic meaning though as we want to be able to update those values and change custom implementations.
  2. Use the <Box> component. This will not support every design or use experience but you can build a lot with it.

I also would strongly recommend folks who do want custom CSS for specific components to let us know what you are trying to do. Potentially the custom CSS could be added back into the system and you will no longer need it.

@vfonic
Copy link
Author

vfonic commented Mar 1, 2024

You may say you care about developers. That doesn't change the fact that, in order to apply custom styles to Polaris components, I usually have to wrap the component into another div and add styles in such a manner (like a bad joke):

.copyCodeSnippet .Polaris-Stack > .Polaris-Stack__Item:first-child {
  flex-grow: 1;
  max-width: 630px;
}
.numberInput--narrow .Polaris-Connected__Item {
  flex-grow: 0;
}

So this just proves that your restriction doesn't work. It just makes my code more difficult to work with.

If there are ~20 comments, tens of 👍, etc. pointing at a certain issue or supporting one point of view (we need className or style) aka "if everyone else is driving in the wrong direction", maybe it's not everyone else, maybe it's you.

Thank you for carrying! ❤️

@alex-page
Copy link
Member

alex-page commented Mar 1, 2024

In those examples above you are overriding layout components from Polaris. I would recommend building a custom layout with our tokens for spacing between elements or using the <Box> component.

@kyledurand
Copy link
Contributor

@vfonic you should use a layout component for that. InlineGrid should do the trick for you

<InlineGrid gap="400" columns="minmax(min-content, 630px) auto">
  <div style={{ background: "lightblue" }} height="320px">
    one
  </div>
  <div style={{ background: "lightblue" }} height="320px">
    two
  </div>
</InlineGrid>

@vfonic
Copy link
Author

vfonic commented Mar 4, 2024

I don't think InlineGrid is available in Polaris v9.9.0. That's the version that I currently use in my apps.
I remember there used to be a place with all the Polaris components listed and in which version of Polaris they were added, but I cannot find it now.
Every few months I go to polaris.shopify.com, and the website (and API) is different. I don't see the need to deprecate, remove, and introduce new (layout) components every few months. The CSS spec didn't change that much in the past few years. And it just proves the point that you cannot possibly cover all the use-cases and layouts. Hence, the freedom to use className and/or style props would come in really handy.

I've counted 5+ components that I use that have been removed in v11.
There are also new components that are called the same like the old component, but are actually completely new components. (??) For example Card has been renamed to LegacyCard and a new Card component has been introduced. I have no idea why and how this is supposed to work and, even with migrator (thank you!), it's gonna take some time to go over all of the pages/components and upgrade.

...and currently, I have a fully working UI that I'm happy with. I just wish that I could style Polaris components in an easier way.

@FarrisIsmati
Copy link

Just add custom styles and be done with it.

@Kamii0909
Copy link

I don't get how supporting customized className and/or style would slow down Polaris development. It might be relevant for your Shopify usecase (to standardize styles across your system), but totally irrelevant to a component library.

You could always allow extensions, but refuse to support any bugs from arbitrary usage of them. You dont even need to read the issue, you could just straight up close them. For your custom usecase, you could always have a custom lint rule that disallow the usage of className (or sx, css, style,...) in your private codebase. What's harder? A lint rule, or human code review for unneccessary div because people can't recall a specific component from their mind?

I see this as nothing but a dogmatic attempt at standardization. But the programming world showed us that a non universal solution would just create another solution. Polaris is the prime example of "I will create another JS framework to fix this". I agree that Polaris is not a fix all solution, a rather niche component library, but what makes it better than mainstream non niche ones? I would be hard pressed to name any specific feature in Polaris.

Prettier can prevent bikeshedding (and arguably did so terribly), but Polaris prevent good design to manifest. That's 2 different scale.

@alex-page
Copy link
Member

It might be relevant for your Shopify usecase

This is the key problem with this issue. The best thing for Shopify is to not add this feature. Developers want this flexibility but it's a tradeoff that can dissolve trust for users of Shopify applications if they do not look like Shopify.

@Kamii0909
Copy link

Your remark is less than satisafactory. I dont know if Shopify is open sourcing Polaris as a way to evade tax from R&D fund or a way to farm free contributions, but your comment means that Polaris doesn't have to caters the usage pattern of others but Shopify. Which is, legally fine and implicitly true.

Although it will, after all, affect the adoption and contributions. In light of open source spirit, I don't think you has any compelling reason for closing this issue thus far, as it is an issue for Polaris the library, just not an issue in Shopify. Open source but closed contribution is not new nor looked down, but Polaris is not one of them.

For your specific Shopify related usecase, you can ban all styling props on Polaris in your proprietary project. ESLint (or any sensible linting solution) provides that rule. You can even use a custom Polaris with className tripped (more work though).

To be fair, the majority of usecases for custom styling is little about styling and more about layout. Having to wrap a component in a div or relying on ad-hoc selector is not a pretty thing to specify flex: 1. Bloating both the JavaScipt and DOM tree is a great way to kill FCP.

@SealinGp
Copy link

SealinGp commented Jul 17, 2024

emm.... actualy if i want to develop an emmbed shopify app, then polaris fixed style or component can't fulfill my requirements, and can't be changed at the same time, i'll seak for other ui library, then it still change the style of shopify application.

might be relevant for your Shopify usecase

@thangpqgm
Copy link

thangpqgm commented Aug 10, 2024

It might be relevant for your Shopify usecase

This is the key problem with this issue. The best thing for Shopify is to not add this feature. Developers want this flexibility but it's a tradeoff that can dissolve trust for users of Shopify applications if they do not look like Shopify.

The Grid component doesn't even have a way for me to add align-item: flex-end;, which is already available in the CSS display: grid;.

I can't override the token because I don't want it to affect the entire project.

Now I have to wrap the whole thing into a div tag, which breaks the entire current layout and creates more garbage in the git history

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests