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] Restrict tag class scope #702

Merged
merged 2 commits into from
Dec 13, 2023
Merged

[fix] Restrict tag class scope #702

merged 2 commits into from
Dec 13, 2023

Conversation

domq
Copy link
Member

@domq domq commented Dec 12, 2023

This fixes a CSS clash since class="tag" is not exactly rare, as seen for instance on the screenshot below (for Storybook).

@domq domq requested a review from rosamaggi December 12, 2023 10:04
@rosamaggi
Copy link

image

@domq domq requested a review from williambelle December 12, 2023 10:11
Copy link

github-actions bot commented Dec 12, 2023

Unit Test Results

    1 files      1 suites   0s ⏱️
267 tests 250 ✔️ 0 💤   0  17 🔥
267 runs  233 ✔️ 0 💤 17  17 🔥

For more details on these errors, see this check.

Results for commit 093a21b.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Dec 12, 2023

🔎 Download the Backstop report for this pull request (link valid for 90 days):

rosamaggi added a commit to epfl-si/epfl-elements-react that referenced this pull request Dec 12, 2023
This fixes a CSS clash with the 'element.css', as seen for instance on the screenshot in the
pull request epfl-si/elements#702
@domq
Copy link
Member Author

domq commented Dec 12, 2023

🔎 Download the Backstop report for this pull request (link valid for 90 days):

* [backstop-report.zip](https://nightly.link/epfl-si/elements/actions/artifacts/1108741094.zip)

I eyeballed the Backstop report and could only find false positives, such as this one.

image

@williambelle
Copy link
Member

If the storybook code is like this:

<div class="language-jsx css-1lwmlsb" style="white-space: pre;">
  <span class="token punctuation">{</span><span class=""> </span><span class=""> </span><span class="token function">render</span><span class="token punctuation">(</span><span class="token punctuation">)</span><span class=""> </span>
  <span class="token punctuation">{</span><span class=""> </span><span class=""> </span><span class="token keyword control-flow">return</span><span class=""> </span><span class="token tag punctuation">&lt;</span>
  <span class="token tag class-name">Button</span><span class="token tag punctuation">&gt;</span><span class="token plain-text"> </span><span class="token plain-text"> </span><span class="token tag punctuation">&lt;</span>
  <span class="token tag">svg</span><span class="token tag"> </span><span class="token tag attr-name">className</span><span class="token tag attr-value punctuation attr-equals">=</span><span class="token tag attr-value punctuation">"</span>
  <span class="token tag attr-value">icon</span><span class="token tag attr-value punctuation">"</span><span class="token tag"> </span><span class="token tag attr-name">aria-hidden</span>
  <span class="token tag attr-value punctuation attr-equals">=</span><span class="token tag attr-value punctuation">"</span><span class="token tag attr-value">true</span><span class="token tag attr-value punctuation">"</span>
  <span class="token tag punctuation">&gt;</span><span class="token plain-text"> </span><span class="token plain-text"> </span><span class="token tag punctuation">&lt;</span><span class="token tag">use</span><span class="token tag"> </span>
  <span class="token tag attr-name">xlinkHref</span><span class="token tag script language-javascript script-punctuation punctuation">=</span><span class="token tag script language-javascript punctuation">{</span>
  <span class="token tag script language-javascript template-string template-punctuation string">`</span><span class="token tag script language-javascript template-string interpolation interpolation-punctuation punctuation">${</span>
  <span class="token tag script language-javascript template-string interpolation">featherIcons</span><span class="token tag script language-javascript template-string interpolation interpolation-punctuation punctuation">}</span>
  <span class="token tag script language-javascript template-string string">#save</span><span class="token tag script language-javascript template-string template-punctuation string">`</span>
  <span class="token tag script language-javascript punctuation">}</span><span class="token tag"> </span><span class="token tag punctuation">/&gt;</span><span class="token plain-text"> </span><span class="token plain-text"> </span>
  <span class="token tag punctuation">&lt;/</span><span class="token tag">svg</span><span class="token tag punctuation">&gt;</span><span class="token plain-text"> </span><span class="token plain-text"> </span>
  <span class="token tag punctuation">&lt;</span><span class="token tag">span</span><span class="token tag"> </span><span class="token tag attr-name">style</span>
  <span class="token tag script language-javascript script-punctuation punctuation">=</span><span class="token tag script language-javascript punctuation">{</span><span class="token tag script language-javascript punctuation">{</span>
  <span class="token tag script language-javascript"> </span><span class="token tag script language-javascript"> </span><span class="token tag script language-javascript literal-property property">marginLeft</span>
  <span class="token tag script language-javascript operator">:</span><span class="token tag script language-javascript"> </span><span class="token tag script language-javascript string">'5px'</span>
  <span class="token tag script language-javascript"> </span><span class="token tag script language-javascript"> </span><span class="token tag script language-javascript punctuation">}</span>
  <span class="token tag script language-javascript punctuation">}</span><span class="token tag punctuation">&gt;</span><span class="token plain-text">Label</span><span class="token tag punctuation">&lt;/</span>
  <span class="token tag">span</span><span class="token tag punctuation">&gt;</span><span class="token plain-text"> </span><span class="token plain-text"> </span><span class="token tag punctuation">&lt;/</span>
  <span class="token tag class-name">Button</span><span class="token tag punctuation">&gt;</span><span class="token punctuation">;</span><span class=""> </span><span class=""> </span><span class="token punctuation">}</span>
  <span class=""> </span><span class=""></span><span class="token punctuation">}</span>
</div>

and we add span.tag, isn't that equal to what you already have?

@rosamaggi
Copy link

Yes, It doesn't correct the problem in storybook. So I commit directly a fix in the epfl-element-react project:
epfl-si/epfl-elements-react@85f3297

domq pushed a commit to epfl-si/epfl-elements-react that referenced this pull request Dec 12, 2023
This fixes a CSS clash with the 'element.css', as seen for instance on the screenshot in the
pull request epfl-si/elements#702
@domq
Copy link
Member Author

domq commented Dec 12, 2023

If the storybook code is like this: [...]
and we add span.tag, isn't that equal to what you already have?

That is correct, but I still think the PR has merit, as we shouldn't spray all the CSS styles over any and all things that have class="tag". While this indeed doesn't solve the storybook case, this PR applies the narrowest possible containment to .tag styles without breaking the promises stated in the elements documentation.

Copy link
Member

@williambelle williambelle left a comment

Choose a reason for hiding this comment

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

I think we need to add div and p.

Tag input

item: (data, escape) => `<div class="tag tag-primary">${escape(data.text)}</div>`,

Screenshot from 2023-12-12 13-21-22

Graph Search

<p class="tag tag-sm bg-light border-light mb-2"><strong>Concepts</strong></p>

Screenshot from 2023-12-12 13-21-39

@domq
Copy link
Member Author

domq commented Dec 13, 2023

I think we need to add div and p.

Done. Apologies for not spotting that in Backstop.

@domq domq merged commit a174b1c into dev Dec 13, 2023
4 checks passed
@domq domq deleted the fix/tag-class-is-too-wide branch December 13, 2023 17:04
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.

3 participants