Skip to content

Conversation

@lottamus
Copy link
Contributor

Element's HttpOperation Request parameters component is using Mosaic's spacing but JSV is not. This results in a 1px difference between the parameters and the body section. This hurts my eyeballs. 🙈

https://github.com/stoplightio/elements/blob/350e46ae4facb93c42997dd97cd57be98b947b88/packages/elements-core/src/components/Docs/HttpOperation/Parameters.tsx#L38

Screen Shot 2021-11-22 at 9 39 02 PM

Screen Shot 2021-11-22 at 9 38 51 PM

Thus I took it upon myself to use Mosaic consistently within this lib.

@lottamus lottamus requested review from Nezteb and marbemac November 23, 2021 03:44
@lottamus lottamus self-assigned this Nov 23, 2021
@lottamus lottamus requested a review from a team November 23, 2021 03:44
@lottamus lottamus requested a review from P0lip November 23, 2021 03:45
@P0lip
Copy link
Contributor

P0lip commented Nov 23, 2021

We didn't use mosaic for a reason.
#113
Have you confirmed the perf issues are no longer apparent?

@marbemac
Copy link
Contributor

marbemac commented Nov 24, 2021

FWIW, on my macbook rendering the stress test storybook.. I don't know where those numbers were coming from in #113...

master

defaultExpandedDepth = 2, around ~145ms:

Screen Shot 2021-11-24 at 12 10 56 AM

defaultExpandedDepth = 5, around ~540ms:

Screen Shot 2021-11-24 at 12 11 33 AM

this PR

defaultExpandedDepth = 2, around ~200ms (1.37x):

Screen Shot 2021-11-24 at 12 12 26 AM

defaultExpandedDepth = 5, around ~930ms (1.38x):

Screen Shot 2021-11-24 at 12 12 55 AM


IMHO the stress test example with expanded depth of 5 is not a use case we need to serve 🤷‍♂️. We don't ever (or should not, right?) set default expand depth to larger than 2, which means realistically we'll never be rendering anything close to the second example above.

145ms versus 200ms for a stress test style use case is not a huge deal IMHO.

@P0lip P0lip changed the title chore: use mosiac consistently chore: use mosaic consistently Nov 24, 2021
@P0lip
Copy link
Contributor

P0lip commented Nov 24, 2021

145ms versus 200ms for a stress test style use case is not a huge deal IMHO.

I will admit that to me the 1.37x slowdown is a notable one, particularly if we speak of tens or hundreds of ms and take into account the fact we usually don't render JSV alone, but a whole docs page that has its own set of accompanying components.
The UI library that, in this very case, brings nothing but styling and no extra functionality on its own shouldn't introduce any meaningful overhead and this is clearly not the case here.
I'll leave the call to you all, but I'd say we shouldn't keep the status quo and not change anything.

60ms equals almost 4 frames.


To be clear - not saying Mosaic is bad or anything. It's a neat library, I'm just not really convinced we should be trying to make use of it at all costs where we clearly take a perf hit here that could have been avoided.

# Conflicts:
#	src/components/SchemaRow/SchemaRow.tsx
@marbemac
Copy link
Contributor

What's in place now just isn't really a good option - I guess we could look for another way that doesn't involve mosaic, buttt.. 🤷‍♂️. JSV IS using mosaic right now, just a "private" API from mosaic (hardcoded classnames that come from mosaic css).

Check this out though:

With the two changes above (I yalc'd that mosaic PR into JSV), the perf is basically the same as on main branch:

Screen Shot 2021-11-25 at 1 48 09 PM

@lottamus lottamus enabled auto-merge (squash) November 30, 2021 19:31
Copy link
Contributor

@Nezteb Nezteb left a comment

Choose a reason for hiding this comment

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

With the new mosaic memoization this is 👌.

@lottamus lottamus merged commit 0a5e49a into master Nov 30, 2021
@lottamus lottamus deleted the chris/mosaic-padding-constant branch November 30, 2021 20:02
@stoplight-bot
Copy link
Collaborator

🎉 This PR is included in version 4.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants