razorpay / blade

Design System that powers Razorpay
https://blade.razorpay.com
MIT License
483 stars 124 forks source link

Text: Add support for width #1227

Closed cseas closed 1 year ago

cseas commented 1 year ago

Figma

Right now we need to use a redundant wrapper Box component just for defining the width of a text.

<Box width="409px">
  <Text>Sample text</Text>
</Box>
divyanshu013 commented 1 year ago

Could you share screenshot of the section? The figma link is taking to the entire file. I think having a Box wrapper might actually be a better approach here.

Background: we intentionally skipped width based props on non Box components link to RFC. Layout based styling props are common for all components and exposing width based props to most components will break the UI.

cseas commented 1 year ago

exposing width based props to most components

This issue is only for exposing it to Text component. If we can't expose certain props to single components, then we need to refactor the codebase to make this possible. Requirements should drive the code structure, not the other way around.

Excessive DOM size is a well documented performance issue and we shouldn't ignore it just because it's not that bad for us yet. The current Text API forces the devs to build a deeper DOM than is required which is not a good practice.

Screenshot:

Screenshot 2023-05-31 at 1 41 43 PM

Related Code

divyanshu013 commented 1 year ago

Adding it only for Text component would be a deviation from how layout based style props work across all components (it's not about refactoring but about API design). All components have the same layout based props for consistency (ref RFC). If there is a width prop on text, natural inclination is it would also be there on Code, Badge, etc. where it can break the UI.

I understand this is a bit inconvenient - needing a Box anytime you need width* - but I'm leaning towards relying on a Box for this in favor of a consistent API.

Regarding excessive DOM size I don't think it's a valid argument here, we would get performance bottlenecked from various other factors much before wrapping a Text with Box creates a noticeable difference.

saurabhdaware commented 1 year ago

Excessive DOM size is a well documented performance issue and we shouldn't ignore it just because it's not that bad for us yet. The current Text API forces the devs to build a deeper DOM than is required which is not a good practice.

This is true but as the link mentions, chrome flags 800+ nodes as warning and 1400+ nodes as error. Even if we say we have to add 100 unnecessary Box wrappers on one HTML page because of blade, that won't end up as perf issue right?

Also excessive DOM elements don't directly impact real user performance unless it's ridiculously (unrealisitically) too much. E.g. even in current razorpay.com, it shows up as error but our FCP on real user metric is 1.6s which is pretty good. Even if we solve that issue, I don't expect razorpay.com's performance in RUM to jump up and there will be other high impact perf issues to solve before we come to this.

Have also seen websites with 3000+ nodes but being pretty fast so agree with Divyanshu above ^^

Having Box wrapper for such usecases is not bad practice right? since it is meant for layout purpose.

iAmServerless commented 1 year ago

Google guidelines of 1400+ nodes is completely a random number. It makes no sense. There are studies around SEO and interlinking which clearly highlights benefits of more content and more link on pages. Breaching 3000 nodes usually helps drive more traffic to website.

It also means we add more content and links on the page and just not random nodes. It becomes quickly exponential the moment it is added in interlinking related components. Single additional div per link end up adding 100% more nodes on the page. The performance impact is clearly visible on websites with nested navigation structure.

Reflows are another problem. Calculating coordinates of a large tree causes severely downgrade the perceived performance. I am using a low grade Samsung phone and these small improvements make so much difference. (It takes 10 sec in my phone to open phone-pay QR scanner vs 2-3 sec for Paytm QR scanner). These improvements are very important for users using low end devices.

On website we share performance as our OKR. Please consider approach which can help us avoid them.

saurabhdaware commented 1 year ago

Single additional div per link end up adding 100% more nodes on the page

It wouldn't need to be added around every link or every text component no? This was definitely something we considered and this is why we added styled-props and Box component to reduce the number of divs atleast for usecases where you just want to add margins around the component or position the component separately.

The UI mentioned in this issue, wouldn't it have one overall div around the heading, text, buttons anyway? Like the entire group in itself will be a container that wraps everything inside of it no?

Calculating coordinates of a large tree causes severely downgrade the perceived performance.

Unless we're adding 1000+ extra divs of dynamic heights and widths the impact of this will be negligible right? Like I mentioned above, nextjs.org has almost twice the number of nodes and still loads pretty fast.

On website we share performance as our OKR. Please consider approach which can help us avoid them.

In the current razorpay.com's performance, there are no problems with FCP or LCP no? (which wouldn't have been the case if excessive dom nodes had direct impact there). In terms of performance, only FID and INP are looking bad which shouldn't have anything to do with rendering and layout performance.

divyanshu013 commented 1 year ago

I think we're getting too much into micro optimization territory ref imho we're speculating a lot:

Regarding excessive DOM size I don't think it's a valid argument here, we would get performance bottlenecked from various other factors much before wrapping a Text with Box creates a noticeable difference.

This breaks down to one factor:

So, let's discuss against this tradeoff

saurabhdaware commented 1 year ago

either have an inconsistent API by specifically having width* props on typography so we can avoid a Box wrapper in certain cases

I think if we could support it on all typography components, I would've been in favour of adding this because atleast it would've been consistent on typography (E.g. we did this by adding color prop on all Typography components recently)

But we can't have this on all Typography components. E.g.

Don't really agree with having one Box wrapper around Text would have any major consequence on performance so I would be in favour of avoiding impossible states and inconsistency in API.

saurabhdaware commented 1 year ago

Conclusion on this Issue

We decided not to support width property on Text right now.

Reasons

This isn't necessarily a final conclusion, we'll be happy to reopen and reconsider this if we get more such requests and if its an issue that is hindering DX or performance by a lot.

cseas commented 1 year ago

Even in the usecase you mentioned in issue, I would believe the overall container will be Box as well

No, we can't add width to the parent there. Feel free to explore here.

We can't really support width on other components yet

Again, this issue is not about supporting it on all components. Categorising some arbitrary set of CSS values as special and making them available necessarily on all components is something only the Blade library does. I don't know any other design system that follows this philosophy. The consumers never asked for API consistency, it's not like the consumer devs have the common props memorised.

What happens is this:

it should be fine to add Box wrapper for those usecases in particular

Sure, not a big problem. But I see myself learning more and more of these workarounds to comfortably work with Blade. This DX isn't synonymous with anything the devs are familiar with. Instead of making Blade easy to work with, we're making devs learn all the quirks it has to work around them.

We would expect these layout things to be majorly done on Box instead

In hindsight, the Box component isn't really working as intended. Its props are so limited, we see examples of devs making custom Box alternatives instead all the time. Creating an alternative of it is so easy, devs aren't even bothering to report issues with it now, just avoiding it.

When thinking of layout components, we think of something like Stack or Grid, something that makes it easy to build layouts. Box is just a very limited version of a basic styled.div, there's no reason why devs would want to use it and stick to its limited props. This should've been brought up during API discussion maybe but practically without context we don't realise how limiting a component is until we actually put it to use in a new design.

Ideally, the expectation is that a design built with Blade in Figma should be possible to build in code exclusively with Blade components. That's obviously not true since we need to create custom styled divs all the time.

it's in micro-optimization territory

I wouldn't call it an optimization, that would be something a dev would go out of their usual way to improve. In Blade's case, my natural intuition is to put the CSS property on the element itself. It's Blade that's making me add that extra wrapper. So it's a micro-deoptimization that Blade's making us do we wouldn't do otherwise at all. The request is not to enable us to build slightly better applications, it's to stop making us build slightly worse applications.

do share some case studies

There's plenty of information available on how DOM size relates to performance:

Please note that many of these talk about the application's performance after it has loaded, we're talking about the application performance through a user journey, not just page load web vitals. We could add a CSS animation that makes the entire application feel slow without it affecting any web vitals. DOM size is related to style calculation cost and all articles I've seen on this topic agree that's true.

Bringing numbers on these and creating 2 different versions of entire Razorpay applications with and without wrapper divs is too big of an effort to request for one CSS prop. Will look into it if I find a good way of evaluating this, could use suggestions on how to.