Skip to content

Fixes #39276 - Add Stylelint rule to block global css overrides#10968

Open
MariaAga wants to merge 1 commit into
theforeman:developfrom
MariaAga:add-stylelint
Open

Fixes #39276 - Add Stylelint rule to block global css overrides#10968
MariaAga wants to merge 1 commit into
theforeman:developfrom
MariaAga:add-stylelint

Conversation

@MariaAga
Copy link
Copy Markdown
Member

AI assisted PR + description:
This PR is to enforce

Use specific css selectors to avoid conflicts with other plugins or the core.

from the handbook https://theforeman.org/handbook.htmln

  • Upgrades Stylelint from v9 to v17 and replaces stylelint-config-standard with stylelint-config-standard-scss. some inherited cosmetic rules are disabled to focus on preventing unscoped CSS overrides.

  • Adds two custom Stylelint plugins:

    • foreman/no-root-pf-overrides — Flags top-level selectors targeting PF classes (e.g. .pf-v5-c-card { ... }) or bare HTML elements combined with PF classes (e.g. div .pf-v5-c-button { ... }). Selectors scoped under a custom class or ID are allowed (e.g. .my-component .pf-v5-c-card { ... }).

    • foreman/no-bare-element-selectors — Flags top-level rules that only use bare HTML element selectors (e.g. div { ... }, table td { ... }). Selectors scoped under a class or ID are allowed (e.g. a#nav { ... }, .my-class div { ... }).

  • Also adds:
    A Stylelint CI step in js_tests.yml that runs on .scss and .css files under webpack/ and app/
    npm run stylelint for linting Foreman core styles
    npm run stylelint:plugins for linting plugin styles using Foreman's shared config

@MariaAga MariaAga requested a review from a team as a code owner April 29, 2026 14:40
@github-actions github-actions Bot added the UI label Apr 29, 2026
@Lukshio
Copy link
Copy Markdown
Contributor

Lukshio commented Apr 29, 2026

Thanks for this PR, hopefully it will solve some of the css overriding issues. Changes looks good.

Comment on lines 343 to +347
/* override bootstrap-sass
ul, ol {
margin-top: 0px;
margin-bottom: 10px;
}*/
} */
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this commented code can be deleted

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Its a comment to show why the rule is there.
as

  margin-bottom: 0;
  font-weight: normal;

was only added because we import bootstrap-sass, which has globally set

    margin-top: 0px;
    margin-bottom: 10px;

@MariaAga
Copy link
Copy Markdown
Member Author

updated dev docs to include stylelint

Copy link
Copy Markdown
Contributor

@kmalyjur kmalyjur left a comment

Choose a reason for hiding this comment

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

Great addition!
I'm just not sure about those things:

if (!HTML_ELEMENTS.has(tag)) return false;

const afterTag = trimmed.slice(tag.length);
if (!afterTag || /^[\s>#~+,{]/.test(afterTag) || afterTag.startsWith('[')) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could # symbol in the regex accidentally trigger false positives for ID selectors like a#nav .pf-v5-c-button?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

a#nav .pf-v5-c-button is allowed, and the lint doesnt show an error for it


min-width: 0;
margin: 0;
--pf-v5-c-menu__list-item--hover--BackgroundColor: var(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How come this could be deleted? 🤔

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

its duplicate, its also defined the same a few lines lower

@MariaAga
Copy link
Copy Markdown
Member Author

rebased

@ofedoren
Copy link
Copy Markdown
Member

Note: This review was AI-generated by Claude (Opus 4.6).

Nice work on enforcing scoped selectors — the approach is solid and the plugin linter integration is a great addition. A few concerns:

  1. Deleted .pf-gray-background class (HostDetails.scss) — this class is removed entirely. If any plugin or ERB template references it, they silently lose their background. Worth grepping across the plugin ecosystem before merging.

  2. Custom lint plugins use regex string-splitting instead of postcss-selector-parser — the isBareElementSelector function splits on /\s*[>~+\s]\s*/ which can produce empty strings for edge-case selectors, and parts.every(...) returns true for an empty array. postcss-selector-parser is already a transitive dep of Stylelint and would handle these cases correctly.

  3. ColumnSelector.js component change bundled with a lint PR — swapping <div className="pf-v5-c-input-group"> for proper <InputGroup>/<InputGroupItem> is a real behavior change that could break plugin snapshot tests. Might be cleaner as a separate PR so this one stays pure infrastructure.

@MariaAga
Copy link
Copy Markdown
Member Author

Thanks!
Added test to the script for clarity, and to make sure any future changes will keep the script working as intented.

Deleted .pf-gray-background class (HostDetails.scss) — this class is removed entirely. If any plugin or ERB template references it, they silently lose their background. Worth grepping across the plugin ecosystem before merging.

pf-gray-background not used anywhere.

Custom lint plugins use regex string-splitting instead of postcss-selector-parser — the isBareElementSelector function splits on /\s*[>~+\s]\s*/ which can produce empty strings for edge-case selectors, and parts.every(...) returns true for an empty array. postcss-selector-parser is already a transitive dep of Stylelint and would handle these cases correctly.

Refactored isBareElementSelector

ColumnSelector.js component change bundled with a lint PR — swapping

for proper / is a real behavior change that could break plugin snapshot tests. Might be cleaner as a separate PR so this one stays pure infrastructure.

Not a really big change, the toolbar should use PF5 components that fit, and InputGroup doesnt really have any code, its a div with css classes.
I checked it and it looks the same.
AI detailed answer:
Looking at the actual source:

  • InputGroup renders <div className="pf-v5-c-input-group">— the exact same class the old
    used manually.
  • InputGroupItem renders <div className="pf-v5-c-input-group__item"> — the old code used pf-v5-c-select-input which is a different class.
    So InputGroup itself is not a behavior change — it produces the same pf-v5-c-input-group div. But replacing the outer
    with (pf-v5-c-input-group__item) is a markup change — different class name, which would break snapshot tests. The behavior is likely identical in practice since neither class adds JS logic, but the rendered HTML does differ.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants