Skip to content

Fix SLDS Linter Errors for SLDS2 #491

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

Open
wants to merge 8 commits into
base: support-slds-2
Choose a base branch
from

Conversation

msmx-mnakagawa
Copy link
Collaborator

@msmx-mnakagawa msmx-mnakagawa commented Jul 3, 2025

What I did

  • avoid to apply the following deprecated classnames
    • .slds-icon-utility-close
    • .slds-icon-utility-error
    • .slds-icon-utility-warning
  • remove unnecessary icon-bare type
  • replace deprecated margin and padding utilities
  • replace deprecated .slds-disabled-text with .slds-color__text_gray-6
  • avoid applying unused classname .slds-icon_medium

How I detected Linter errors

  1. extract markups from all the stories as *.html files
  2. apply SLDS Linter, by referring to https://developer.salesforce.com/docs/platform/slds-linter/guide/get-started-setup-linter.html
  3. open the *.html files in VSCode and confirm whether VSCode shows linter errors

Classnames I did NOT change despite Linter errors

The followings are also reported as lint errors.
However, I kept them as is intentionally, because they are documented as the current markup examples.
In addition, particularly about .slds-form_… classnames, we allow to use them as exceptions.

  • .slds-is-required
  • .slds-dropdown_top
  • .slds-dropdown__list
  • .slds-form_compound
  • .slds-form_inline
  • .slds-listbox__option-text
  • .slds-listbox__option-meta_entity
  • .slds-alert_success
  • .slds-page-header__icon

Reference

Deprecated classname list
[
  "slds-action-overflow_touch",
  "slds-action-overflow_touch__body",
  "slds-action-overflow_touch__container",
  "slds-action-overflow_touch__content",
  "slds-action-overflow_touch__footer",
  "slds-action-overflow--touch",
  "slds-action-overflow--touch__body",
  "slds-action-overflow--touch__container",
  "slds-action-overflow--touch__content",
  "slds-action-overflow--touch__footer",
  "slds-app-launcher__tile_small",
  "slds-app-launcher__tile--small",
  "slds-app-launcher__tile-body_small",
  "slds-app-launcher__tile-body--small",
  "slds-app-launcher__tile-figure_small",
  "slds-app-launcher__tile-figure--small",
  "slds-attachments__item",
  "slds-chat-avatar__intials",
  "slds-chat-event__rule",
  "slds-checkbox_stacked",
  "slds-comment__replies",
  "slds-context-bar_theme-marketing",
  "slds-context-bar--theme-marketing",
  "slds-datepicker_time",
  "slds-datepicker_time__list",
  "slds-datepicker--time",
  "slds-datepicker--time__list",
  "slds-dropdown_nubbin-top",
  "slds-dropdown--nubbin-top",
  "slds-faux-input",
  "slds-faux-input_label",
  "slds-flip_horizontal",
  "slds-flip_vertical",
  "slds-flip--horizontal",
  "slds-flip--vertical",
  "slds-form-element__static_edit",
  "slds-global-header__button_icon",
  "slds-global-header__button_icon-actions",
  "slds-global-header__button_icon-favorites",
  "slds-global-header__button--icon",
  "slds-global-header__button--icon-actions",
  "slds-global-header__button--icon-favorites",
  "slds-global-header__icon",
  "slds-global-search__form-element",
  "slds-has-icon",
  "slds-has-icon_left-right",
  "slds-has-icon_right",
  "slds-has-icon--left-right",
  "slds-has-icon--right",
  "slds-icon__container",
  "slds-icon__container_circle",
  "slds-icon__container--circle",
  "slds-icon_left",
  "slds-icon_right",
  "slds-icon--left",
  "slds-icon--right",
  "slds-icon-utility-close",
  "slds-icon-utility-error",
  "slds-icon-utility-warning",
  "slds-is-focused",
  "slds-kx-is-animating-from-click",
  "slds-media_timeline",
  "slds-media--timeline",
  "slds-mobile-combobox",
  "slds-mobile-combobox__addon",
  "slds-mobile-combobox__header",
  "slds-mobile-combobox__header-has-icon",
  "slds-mobile-combobox__input",
  "slds-mobile-lookup__listbox",
  "slds-mobile-lookup__listbox_container",
  "slds-mobile-lookup__listbox_icon_container",
  "slds-mobile-lookup__listbox_loader",
  "slds-mobile-lookup__listbox_meta",
  "slds-mobile-lookup__listbox_text",
  "slds-mobile-lookup__listbox_trigger",
  "slds-mobile-lookup__listbox-option",
  "slds-mobile-lookup__listbox-option_heading",
  "slds-modal_form",
  "slds-modal--form",
  "slds-notify-container",
  "slds-popover_feature",
  "slds-popover_walkthrough",
  "slds-popover_walkthrough-alt",
  "slds-popover--walkthrough",
  "slds-section-group_is-closed",
  "slds-section-group--is-closed",
  "slds-section-title",
  "slds-styling-hooks",
  "slds-tabs__content",
  "slds-tabs__item",
  "slds-tabs__nav-scroller",
  "slds-tabs__nav-scroller_inner",
  "slds-tabs__nav-scroller--inner",
  "slds-tabs_default__header",
  "slds-tabs_mobile",
  "slds-tabs_mobile__item",
  "slds-tabs_mobile__title",
  "slds-tabs_mobile__title-action",
  "slds-tags",
  "slds-tags__item",
  "slds-tags__list",
  "slds-text-heading_label-normal",
  "slds-text-heading--label-normal",
  "slds-theme_alert-texture",
  "slds-theme--alert-texture",
  "slds-timeline__media",
  "slds-timeline__media_call",
  "slds-timeline__media_email",
  "slds-timeline__media_event",
  "slds-timeline__media_task",
  "slds-timeline__media--call",
  "slds-timeline__media--email",
  "slds-timeline__media--event",
  "slds-timeline__media--task",
  "slds-timeline__title",
  "slds-timeline__title-content"
]

@msmx-mnakagawa msmx-mnakagawa self-assigned this Jul 3, 2025
Copy link

reg-suit bot commented Jul 3, 2025

reg-suit detected visual differences.

Check this report, and review them.

🔴🔴

🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵

What do the circles mean? The number of circles represent the number of changed images.
🔴 : Changed items, ⚪ : New items, ⚫ : Deleted items, and 🔵 Passed items

How can I change the check status? If reviewers approve this PR, the reg context status will be green automatically.

@msmx-mnakagawa msmx-mnakagawa mentioned this pull request Jul 3, 2025
44 tasks
@msmx-mnakagawa msmx-mnakagawa changed the base branch from support-slds-2-icon-button to support-slds-2 July 10, 2025 02:22
@msmx-mnakagawa msmx-mnakagawa force-pushed the support-slds-2-apply-slds-linter branch from 1d3dc30 to 0098345 Compare July 10, 2025 02:25
@msmx-mnakagawa msmx-mnakagawa force-pushed the support-slds-2-apply-slds-linter branch 2 times, most recently from ce9e951 to 276c09a Compare July 10, 2025 06:06
@msmx-mnakagawa msmx-mnakagawa force-pushed the support-slds-2-apply-slds-linter branch from 92501b8 to 11b4bdb Compare July 11, 2025 01:21
@msmx-mnakagawa msmx-mnakagawa requested a review from stomita July 11, 2025 02:49
@msmx-mnakagawa msmx-mnakagawa marked this pull request as ready for review July 11, 2025 02:49
@@ -246,7 +246,7 @@ const DatepickerDate: FC<DatepickerDateProps> = (props) => {
const isToday = date.value === today;
const isAdjacentMonth = date.month !== cal.month;
const dateClassName = classnames({
'slds-disabled-text': !enabled,
'slds-color__text_gray-6': !enabled && !isAdjacentMonth,
Copy link
Collaborator

Choose a reason for hiding this comment

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

It should be shown in disabled even if it is in adjacent month. As the adjacent-month class assumes the day is clickable, it should not be shown as clickable if the day is not enabled.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@stomita
IMO, slds-color__text_gray-6 would keep affording that date elements having it are disabled, because it has grayed style.

In addition, slds-disabled-text is listed as deprecated class in the spec.
https://v1.lightningdesignsystem.com/components/datepickers/#CSS-Class-Overview

To me, it'd be better to avoid using deprecated slds-disabled-text and use slds-color__text_gray-6 that keeps disabled affordance.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@msmx-mnakagawa
In the storybook min/max story example, it uses slds-day_adjacent-month for out of range (= disabled) days. (Not sure that is proper way considering the meaning, though).
https://sds-site-docs-1fea39e7763a.herokuapp.com/index.html?path=/story/components-datepicker--min-max

[`slds-icon-text-${textColor ?? 'default'}`]:
/^(default|success|warning|error|light)$/.test(textColor ?? '') &&
!iconColor,
'slds-m-left_x-small': align === 'right',
'slds-m-right_x-small': align === 'left',
'slds-var-m-left_x-small': align === 'right',
Copy link
Collaborator

@stomita stomita Jul 13, 2025

Choose a reason for hiding this comment

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

Is there any reason to change them to var- classes ? Do you think it should change value in comfy/compact mode ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same as #491 (comment).

@@ -277,7 +281,8 @@ export const Icon = createFC<IconProps, { ICONS: typeof ICONS }>(
containerClassName_,
'slds-icon_container',
container === 'circle' ? 'slds-icon_container_circle' : null,
category === 'utility'
category === 'utility' &&
!UTILITY_ICONS_DEPRECATED_FOR_CONTAINER_CLASSNAME.includes(icon)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Describe the reason of why the deprecated icon list is introduced.

Copy link
Collaborator Author

@msmx-mnakagawa msmx-mnakagawa Jul 18, 2025

Choose a reason for hiding this comment

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

@stomita
Sorry for my lack of explanation...

After installing @salesforce-ux/eslint-plugin-slds into our local environment, we can see the list of deprecated classnames at node_modules/@salesforce-ux/sds-metadata/generated/deprecatedClasses.json.

The repository itself seems existed, but we need authorization to view it.
https://github.com/salesforce-ux-emu/design-systems-metadata

Therefore, I put the list I got to the summary of this PR.
The list contains classnames I avoided using here.

Copy link
Collaborator

@stomita stomita Jul 22, 2025

Choose a reason for hiding this comment

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

@msmx-mnakagawa Is it necessary to be managed by the list ? The icon string can be an arbitrary one, both the deprecated and totally invalid, so changing the behavior for deprecated class names is out of the point (always the valid icon notation will work properly).

@@ -100,13 +100,13 @@ export const Sizes: StoryObj<StoryProps> = {
}) => (
<div>
<Icon icon='standard:case' size='xx-small' onClick={xxsmall_onClick} />
<span className='slds-p-right_small' />
<span className='slds-var-p-right_small' />
Copy link
Collaborator

Choose a reason for hiding this comment

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

The var- class name should be introduced carefully that is not changing the layout in comfy/compact mode.

Copy link
Collaborator Author

@msmx-mnakagawa msmx-mnakagawa Jul 18, 2025

Choose a reason for hiding this comment

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

@stomita
As a result of our discussion, at least on the package side, we concluded we don't need these classnames.
This is because the package has no way to know which the current layout is.

I reverted the replacement in 0cd2296.

@msmx-mnakagawa msmx-mnakagawa requested a review from stomita July 22, 2025 00:35
@@ -246,7 +246,7 @@ const DatepickerDate: FC<DatepickerDateProps> = (props) => {
const isToday = date.value === today;
const isAdjacentMonth = date.month !== cal.month;
const dateClassName = classnames({
'slds-disabled-text': !enabled,
'slds-color__text_gray-6': !enabled && !isAdjacentMonth,
Copy link
Collaborator

Choose a reason for hiding this comment

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

@msmx-mnakagawa
In the storybook min/max story example, it uses slds-day_adjacent-month for out of range (= disabled) days. (Not sure that is proper way considering the meaning, though).
https://sds-site-docs-1fea39e7763a.herokuapp.com/index.html?path=/story/components-datepicker--min-max

@@ -277,7 +281,8 @@ export const Icon = createFC<IconProps, { ICONS: typeof ICONS }>(
containerClassName_,
'slds-icon_container',
container === 'circle' ? 'slds-icon_container_circle' : null,
category === 'utility'
category === 'utility' &&
!UTILITY_ICONS_DEPRECATED_FOR_CONTAINER_CLASSNAME.includes(icon)
Copy link
Collaborator

@stomita stomita Jul 22, 2025

Choose a reason for hiding this comment

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

@msmx-mnakagawa Is it necessary to be managed by the list ? The icon string can be an arbitrary one, both the deprecated and totally invalid, so changing the behavior for deprecated class names is out of the point (always the valid icon notation will work properly).

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.

2 participants