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: change the naming of status indicator classes to follow BEM #2204

Merged
merged 5 commits into from Mar 22, 2021

Conversation

Lokanathan-k
Copy link
Contributor

@Lokanathan-k Lokanathan-k commented Mar 18, 2021

Related Issue

Closes SAP/fundamental-styles#

Description

changing the BEM to the status indicator and adding link css

Screenshots

NOTE: If you've made any style changes, please provide appropriate screenshots (before and after) to help reviewers.

To see examples of which screenshots to include, go to Screenshot Examples.

Before:

fd-status-indicator--lg__text and fd-status-indicator--critical__text.

After:

fd-status-indicator__text--lg fd-status-indicator__text--critical

Please check whether the PR fulfills the following requirements

  1. The output matches the design specs
  • [NA] All values are in rem
  • [NA] Text elements follow the truncation rules
  • [NA] hover state of the element follow design spec
  • [NA] focus state of the element follow design spec
  • [NA] active state of the element follow design spec
  • [NA] selected state of the element follow design spec
  • [NA] selected hover state of the element follow design spec
  • [NA] pressed state of the element follow design spec
  • [NA] Responsiveness rules - the component has modifier classes for all breakpoints
  • [NA] Includes Compact/Cosy/Tablet design
  • [NA] RTL support
  1. The code follows fundamental-styles code standards and style
  • [NA] only one top level fd-* class is used in the file
  • BEM naming convention is used
  • Mixins are used for repeatable code (fd-rtl, fd-ellipsis, fd-flex, fd-selected, fd-focus, ect.)
  • [NA] A11y support - keyboard support, screenreader support, proper ARIA attributes, etc.
  • [NA] fd-reset() mixin is applied to all elements
  • [NA] Variables are used, if some value is used more than twice.
  • [NA] Checked if current components can be reused, instead of having new code.
  1. Testing
  • [NA] tested Storybook examples with "CSS Resources" normalize option
  • [NA] tested Storybook examples with "CSS Resources" unnormalize option
  • [NA] Verified all styles in IE11
  • [NA] Updated tests
  • [NA] last commit message should have [ci visual] so it can trigger chromatic visual regression (e.g. test: run chromatic visual regression [ci visual])
  1. Documentation
  • [NA] Storybook documentation has been created/updated
  • Breaking Changes wiki has been updated in case of breaking changes.

changing the BEM to the status indicator and adding link css
@Lokanathan-k Lokanathan-k added the Bug Something isn't working label Mar 18, 2021
@Lokanathan-k Lokanathan-k self-assigned this Mar 18, 2021
@netlify
Copy link

netlify bot commented Mar 18, 2021

Deploy preview for fundamental-styles ready!

Built with commit 819d615

https://deploy-preview-2204--fundamental-styles.netlify.app

Copy link
Contributor

@JKMarkowski JKMarkowski left a comment

Choose a reason for hiding this comment

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

Hello @Lokanathan-k Could you add [ci visual] text to your commits message? Just to trigger chromatic. Also you need to indicate that it's BREAKING CHANGE.

Copy link
Contributor

@InnaAtanasova InnaAtanasova left a comment

Choose a reason for hiding this comment

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

@Lokanathan-k , you checked in the template that you updated the breaking changes in the wiki but you didn't. The checks in the PR templates are not there just to be checked :)

@InnaAtanasova InnaAtanasova changed the title fix: changing the BEM of status indicaytor from fd-status-indicator--lg__text to fd-status-indicator__text--lg fix: change the naming of status indicator classes to follow BEM Mar 18, 2021
@InnaAtanasova
Copy link
Contributor

InnaAtanasova commented Mar 18, 2021

Also there's something wrong with the BEM structure in the case of Status indicator with Label.
Currently you have the following structure:

<div class="fd-status-indicator fd-status-indicator--critical fd-status-indicator--htext">
    <span class="fd-status-indicator__text--lg fd-status-indicator__text--critical">100%</span>
    <svg class="fd-status-indicator--lg" ...>....
....
</div>
  1. What is fd-status-indicator--htext? (maybe name it horizontal-label)
  2. Why --lg modifiers are put on the span level and on the svg level? In the other examples the size modifiers are applied on the parent level: fd-status-indicator fd-status-indicator--lg.
  3. You are repeating the critical modifier on the parent level and on the child level. You can't have fd-status-indicator--critical and fd-status-indicator__text--critical. It's redundant and not a good practice.
  4. Your svg has <svg class="fd-status-indicator--lg" ...> this is not possible. You can't have a modifier class without a base class.
  5. fd-status-indicator__text should be fd-status-indicator__label as it makes more sense

The structure should be something like:

<div class="fd-status-indicator fd-status-indicator--critical fd-status-indicator--lg">
    <span class="fd-status-indicator__label">100%</span>
    <svg class="fd-status-indicator__svg" ...>....
....
</div>

then you rely on specificity to overwrite the children properties:

.fd-status-indicator--critical {
    .fd-status-indicator__label {...}
    .fd-status-indicator__svg {...}
}

and

.fd-status-indicator--lg {
    .fd-status-indicator__label {...}
    .fd-status-indicator__svg {...}
}

@Lokanathan-k
Copy link
Contributor Author

Also there's something wrong with the BEM structure in the case of Status indicator with Label.
Currently you have the following structure:

<div class="fd-status-indicator fd-status-indicator--critical fd-status-indicator--htext">
    <span class="fd-status-indicator__text--lg fd-status-indicator__text--critical">100%</span>
    <svg class="fd-status-indicator--lg" ...>....
....
</div>
  1. What is fd-status-indicator--htext? (maybe name it horizontal-label): Changed
  1. Why --lg modifiers are put on the span level and on the svg level? In the other examples the size modifiers are applied on the parent level: fd-status-indicator fd-status-indicator--lg.

on using with the parent level the svg size gets reduced on add the label content. or the label has to be kept as a separate component. since the label size vary from sm to xl and the margin size is different for different size.

  1. You are repeating the critical modifier on the parent level and on the child level. You can't have fd-status-indicator--critical and fd-status-indicator__text--critical. It's redundant and not a good practice: changed
  1. Your svg has <svg class="fd-status-indicator--lg" ...> this is not possible. You can't have a modifier class without a base class.
    on add this on the parent tag found that the images size reduces and becomes distorted. on adding it to the respective SVG element we can see the image as defined in the spec
  1. fd-status-indicator__text should be fd-status-indicator__label as it makes more sense: Changed

The structure should be something like:

<div class="fd-status-indicator fd-status-indicator--critical fd-status-indicator--lg">
    <span class="fd-status-indicator__label">100%</span>
    <svg class="fd-status-indicator__svg" ...>....
....
</div>

then you rely on specificity to overwrite the children properties:

.fd-status-indicator--critical {
    .fd-status-indicator__label {...}
    .fd-status-indicator__svg {...}
}

and

.fd-status-indicator--lg {
    .fd-status-indicator__label {...}
    .fd-status-indicator__svg {...}
}

BEM change to the css classes and adding testing storyshots
@InnaAtanasova
Copy link
Contributor

InnaAtanasova commented Mar 19, 2021

@Lokanathan-k ,
You can't have a structure like <span class="fd-status-indicator__label--lg">100%</span> or <svg class="fd-status-indicator--lg"...>. That's just not possible. Classes containing --something are MODIFIER classes, they modify something and are NEVER used alone. They are supposed to modify a property or properties that belong to the element class applied on the same level.
So, if you have a class .fd-status-indicator__label that for example has color red and font-size 14px you will have:

.fd-status-indicator__label {
    color: red;
    font-size: 14px
}

Then in case you want to have a label that has bigger font-size you create a MODIFIER class and overwrite ONLY the font-size:

.fd-status-indicator__label--lg {
    font-size: 16px
}

But you need to always apply both, because if you apply only .fd-status-indicator__label--lg the element will not get the red color defined in the element class:

<h4 fd-status-indicator__label fd-status-indicator__label--lg>something</h4>

updating the BEM and test cases [ci visual]
<span class="fd-status-indicator--lg__text fd-status-indicator--critical__text">100%</span>
<svg id="__shape0__box9-24" class="fd-status-indicator--lg" data-sap-ui="__shape0-__box21-24" version="1.1" xlmns="http://www.w3.org/2000/svg" viewBox="0 0 26 25" preserveAspectRatio="xMidYMid meet" x="0" y="0" width="100%" height="100%">
<div class="fd-status-indicator fd-status-indicator--critical fd-status-indicator--lg" aria-roledescription="Status Indicator" role="progressbar" aria-valuetext="100%" tabindex=0 aria-label="Euro Status Indicator With Labelled On Top" focusable="true" title="100% with label on top">
<span class="fd-status-indicator__label--lg">100%</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be:

<span class="fd-status-indicator__label fd-status-indicator__label--lg">100%</span>

@@ -330,14 +330,14 @@ export const StatusIndicatorLabels = () => `
</path>
</svg>
</svg>
<span class="fd-status-indicator--lg__text fd-status-indicator--positive__text">100%</span>
<span class="fd-status-indicator__label--lg">100%</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be:

<span class="fd-status-indicator__label fd-status-indicator__label--lg">100%</span>

<span class="fd-status-indicator--lg__text fd-status-indicator--critical__text">100%</span>
<svg id="__shape0__box12-24" class="fd-status-indicator--lg" data-sap-ui="__shape0-__box21-24" version="1.1" xlmns="http://www.w3.org/2000/svg" viewBox="0 0 36 25" preserveAspectRatio="xMidYMid meet" x="0" y="0" width="100%" height="100%">
<div class="fd-status-indicator fd-status-indicator--critical fd-status-indicator--horizontal-label fd-status-indicator--lg" aria-roledescription="Status Indicator" role="progressbar" aria-valuetext="100%" tabindex=0 aria-label="Euro Status Indicator With Labelled On Left" focusable="true" title="100% with label on left">
<span class="fd-status-indicator__label--lg">100%</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be:

<span class="fd-status-indicator__label fd-status-indicator__label--lg">100%</span>

@@ -396,7 +396,7 @@ export const StatusIndicatorLabels = () => `
</path>
</svg>
</svg>
<span class="fd-status-indicator--lg__text fd-status-indicator--positive__text">100%</span>
<span class="fd-status-indicator__label--lg">100%</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be:

<span class="fd-status-indicator__label fd-status-indicator__label--lg">100%</span>

we already discussed that a modifier class like fd-status-indicator__label--lg should not be used alone but with a base element class like fd-status-indicator__label. In this case fd-status-indicator__label--lg is modifying the props set in fd-status-indicator__label.

@InnaAtanasova InnaAtanasova dismissed JKMarkowski’s stale review March 22, 2021 21:57

sorry to dismiss, but need to merge this PR

@InnaAtanasova InnaAtanasova merged commit 94f3bb1 into main Mar 22, 2021
@InnaAtanasova InnaAtanasova deleted the StatusIndicator_bugFix branch March 22, 2021 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
Development
  
Awaiting triage
Development

Successfully merging this pull request may close these issues.

None yet

3 participants