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

fix: add explicit styles for welcome tab #4599

Merged
merged 7 commits into from
Oct 15, 2024
Merged

Conversation

jarekdanielak
Copy link
Contributor

Closes #4555

Proposed Changes

Welcome tab now always looks the same, even if you open and close some diagrams.

Checklist

To ensure you provided everything we need to look at your PR:

  • Brief textual description of the changes present
  • Visual demo attached
  • Steps to try out present, i.e. using the @bpmn-io/sr tool
  • Related issue linked via Closes {LINK_TO_ISSUE} or Related to {LINK_TO_ISSUE}

client/src/app/EmptyTab.less Outdated Show resolved Hide resolved
@jarekdanielak
Copy link
Contributor Author

The root cause of this issue are carbon styles user in variable outline. There is no simple way to scope them without some hacking (thanks @marstamm), so I'll stick with explicit styles on the side of welcome page.

I realized there is no need to introduce new class as I can just reuse tab__name for welcome page too. Having p font-size defined explicitly doesn't seem like a hack for me. We want the font on this page to be this size, so it might as well say it in the styles.

Copy link
Member

@nikku nikku left a comment

Choose a reason for hiding this comment

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

Great root causing on this issue! What we can see is that the carbon normalization (that is lazy loaded!) has many effects, and will always override whatever we normalize in the modeler.

What I propose is that we tackle this in an orderly manner as part of #4511, which I've moved to ready.

Before

screenshot z0538j

After

image

Root cause

(Again, Carbon "normalization"):

capture g1Kdt0_optimized

@nikku nikku force-pushed the welcome-tab-styles branch 2 times, most recently from 7d16afb to af6af15 Compare October 15, 2024 09:25
@nikku
Copy link
Member

nikku commented Oct 15, 2024

For now I pushed a bunch of additional fixes:

  • e375843 => We not only fix font-size, but also paragraph styles.
  • 0f4a4ad => We fix custom toggle formatting
  • 196c6b0 => We fix line height in all UI heavy components

Some screenshots (fixed, broken in Camunda Modeler v5.28.0) attached:

screenshot 6axQ93
screenshot X6wTOe
screenshot ks3Hgt
screenshot dG4O3o
screenshot lqJ22i

@nikku nikku requested a review from lmbateman October 15, 2024 11:49
Copy link
Member

@nikku nikku left a comment

Choose a reason for hiding this comment

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

Ready to be merged from my end (#4599 (comment)).

@jarekdanielak jarekdanielak merged commit 4191232 into main Oct 15, 2024
11 of 12 checks passed
@jarekdanielak jarekdanielak deleted the welcome-tab-styles branch October 15, 2024 13:36
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Oct 15, 2024
@github-actions github-actions bot added this to the M82 milestone Oct 15, 2024
@lmbateman
Copy link

lmbateman commented Oct 15, 2024

Not sure I have anything constructive to add, except maybe that the radio buttons in the Deploy popup look very odd (at least on my machine). Many thanks to Nico for looping me in and helping me to understand the line height issues.
image

@nikku
Copy link
Member

nikku commented Oct 15, 2024

@lmbateman Let's verify on tomorrows nightly if they look odd still. Should be fixed with this PR.

@barmac
Copy link
Collaborator

barmac commented Oct 16, 2024

Still reproducible:
image

@nikku
Copy link
Member

nikku commented Oct 16, 2024

Yea. Was merged to main, pending integration into develop, which is done now.

Fixed in tomorrows nightly 🙃, or on develop:

image

@barmac
Copy link
Collaborator

barmac commented Oct 17, 2024

Confirmed:
image

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

Successfully merging this pull request may close these issues.

4 participants