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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(tooltip): Fix Tooltip selectors specificity #2885

Closed
wants to merge 1 commit into from
Closed

fix(tooltip): Fix Tooltip selectors specificity #2885

wants to merge 1 commit into from

Conversation

robertjk
Copy link

@robertjk robertjk commented May 7, 2022 •
edited

Description

There is a bug in EndUserApp with improper padding-right value applied on <GuideTooltip>. It's caused by accidental change of order of CSS styles from <GuideTooltip> and <Tooltip> components.

Currently right padding on a <GuideTooltip> with close button has 28px (visible below):

Screenshot 2022-05-07 at 19-35-46 All Files Powered by Box

The intended value from how the styles are written should be 48px (visible below):

Screenshot 2022-05-07 at 19-34-44 All Files Powered by Box

The current value is different than intended because box-content-preview imports <Tooltip> component (more details below).

It's not clear to me if it should be fixed in box-ui-elements or in EndUserApp, nor what the padding's value should be. For now I'm posting a Draft PR here with intention of having discussion about it.

Nitty-gritty

padding-right value is declared in 2 places:

// Tooltip.scss (imported first)
.bdlTooltip.with-close-button {
    padding-right: 28px;
}

// GuideTooltip.scss (imported later)
.bdl-GuideTooltip.bdl-Tooltip {
    padding: ($bdl-grid-unit * 6) ($bdl-grid-unit * 12) ($bdl-grid-unit * 6) ($bdl-grid-unit * 6);
}

Intended flow

  • .bdl-GuideTooltip.bdl-Tooltip and .bdl-Tooltip.with-close-button match the same element for Guide Tooltip with close button: Screenshot 2022-05-07 at 18 36 04
  • Both selectors have the same specificity (2 classes)
  • GuideTooltip.scss is imported later than Tooltip.scss (as <GuideTooltip> imports <Tooltip>), so it takes precedence

The result is that the padding should have 48px.

Current flow in EUA

EndUserApp imports box-content-preview and it's styles in preview.css. As box-content-preview imports <Tooltip> (but not <GuideTooltip>) component from box-ui-elements, preview.css stylesheet has it's styles duplicated. As preview.css comes later in EUA's <head> than main EUA's styles, Tooltip.scss styles are effectively later in declaration order than GuideTooltip.css, even though they should be earlier.

<!-- EndUserApp -->
<head>
  <link href="src_mainAfterPolyfill_js.css" />
    * includes Tooltip.scss
    * includes GuideTooltip.scss
  <link href="preview.css" />
    * includes Tooltip.scss again
 </head>

The result is that the padding has 28px.

I noticed the bug, when I imported <GuideTooltip> in box-content-preview and the padding on tooltips changed for no obvious reason. Took me quite a while to identify the problem.

Which padding value is intended?

First I guess we should answer which padding value is the intended one? It looks like it should be 48px, but it has been 28px for a while now (a year it seems) and maybe that's what we actually prefer... 馃

Possible solutions

Below I propose 2 different solutions to fix this problem. I assumed 48px is intended value, but they can be modified to use 28px instead.

1. Include preview.css earlier in EndUserApp

One solution is changing the order of CSS declarations in EndUserApp's <head> so that preview.css comes before main EndUserApp's styles. That would fix the problem both for those tooltip components and any other similar problems that likely already exist or would appear in the future.

But it could bring a new array of problems, as we would change order of quite a lot of rules. Different rules might start getting applied to what was applied before in different places of the app.

2. Use !important

Using !important is definitely not a good CSS practice, but it would solve the problem here and be safe to not break other things. It would make further extensions of <GuideTooltip> harder though.

// Tooltip.scss (imported first)
.bdlTooltip.with-close-button {
    padding-right: 28px;
}

// GuideTooltip.scss (imported later)
.bdl-GuideTooltip {
    padding: ($bdl-grid-unit * 6) ($bdl-grid-unit * 12) ($bdl-grid-unit * 6) ($bdl-grid-unit * 6) !important;
}

Specificity of .bdl-GuideTooltip.bdl-Tooltip selector

There is one more thing worth to talk about: It's not a good practice that .bdl-GuideTooltip.bdl-Tooltip selector uses 2 classes to target an element instead of 1. Those 2 classes will be always applied together (.bdl-GuideTooltip is passed as className property to <Tooltip>, which applies it on the same element as has .bdl-Tooltip class). So .bdl-GuideTooltip.bdl-Tooltip and .bdl-GuideTooltip selectors target exactly the same set of elements. There is a good reason why methodologies like BEM exist and recommend always using single class selectors (other than when you have modifiers) and we break that good practice here.

I understand it was probably done to equal the specificity of .bdlTooltip.with-close-button selector, but there is another solution we could use - re-declaring .with-close-button again in GuideTooltip.scss:

// Tooltip.scss (imported first)
.bdlTooltip.with-close-button {
    padding-right: 28px;
}
// GuideTooltip.scss (imported later)
.bdl-GuideTooltip {
    padding: ($bdl-grid-unit * 6) ($bdl-grid-unit * 12) ($bdl-grid-unit * 6) ($bdl-grid-unit * 6);

    &.with-close-button {
        padding-right: ($bdl-grid-unit * 12);
    }
}

@robertjk robertjk requested a review from a team as a code owner May 7, 2022 16:48
@CLAassistant
Copy link

CLAassistant commented May 7, 2022 •
edited

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

Robert J. Kusznier seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@robertjk robertjk marked this pull request as draft May 7, 2022 17:41
@robertjk robertjk requested a review from a user May 7, 2022 17:53
@robertjk robertjk closed this by deleting the head repository May 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ConradJChan ConradJChan Awaiting requested review from ConradJChan

@jstoffan jstoffan Awaiting requested review from jstoffan

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@robertjk @CLAassistant