Skip to content

Fixes #37904 - move css from vendor to foreman - #10345

Merged
evgeni merged 1 commit into
theforeman:developfrom
MariaAga:move-css-foreman-js
Jun 12, 2025
Merged

Fixes #37904 - move css from vendor to foreman#10345
evgeni merged 1 commit into
theforeman:developfrom
MariaAga:move-css-foreman-js

Conversation

@MariaAga

Copy link
Copy Markdown
Member

One of the steps for removing foreman-js vendor is moving the css into foreman. (#10342).
This PR is not a breaking change, removing theforeman/vendor will be a breaking change.
I will also open PRs for all plugins I found (Ansible,Remote Execution ,Templates,foreman-tasks,Katello,Leapp )
This PR also includes packages/vendor-core/scss/vendor-core.scss and building it in webpack from foreman core instead of building it in webpack 4 in foreman-js

Comment thread webpack/assets/javascripts/react_app/common/variables.scss Outdated
@MariaAga
MariaAga force-pushed the move-css-foreman-js branch 2 times, most recently from c7acb7d to 2f6ac72 Compare March 28, 2025 13:29
@MariaAga
MariaAga force-pushed the move-css-foreman-js branch 3 times, most recently from 2f6ac72 to 85b1beb Compare April 15, 2025 10:59

@ekohl ekohl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The failure:

Capybara::ElementNotFound: Unable to find visible css "#ms-domain_location_ids .ms-selectable"

Have you looked at this?

Comment thread config/webpack.config.js Outdated
Comment thread package.json Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This will need to be packaged.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@MariaAga

Copy link
Copy Markdown
Member Author

Capybara::ElementNotFound: Unable to find visible css "#ms-domain_location_ids .ms-selectable"

Wasnt there before and looks like one of the randomly failing tests, re running it and will check it, thanks!

@MariaAga

Copy link
Copy Markdown
Member Author

re-ran the tests and now its not failing

ShimShtein
ShimShtein previously approved these changes Apr 27, 2025

@ShimShtein ShimShtein left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Created a packaging PR.
@ekohl do we want to merge the packaging PR first and then make sure packit is working correctly?

Comment thread package.json Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@evgeni

evgeni commented Apr 28, 2025

Copy link
Copy Markdown
Member

Packit needs an updated foreman.spec to pull in the new dep, so it will never¹ work in this PR and I think that's OK?

¹: you could have a prepared branch for updating foreman.spec in foreman-packaging and update packit to use that spec file for showing it works, and then reverting back to the vanilla one for the merge.

@ShimShtein

Copy link
Copy Markdown
Member

Sounds like too much trouble and too error prone.
IMHO having the packaging PR merged would reduce the changes of something big being broken after the merge.

ShimShtein
ShimShtein previously approved these changes May 14, 2025
@ShimShtein

Copy link
Copy Markdown
Member

Please rebase this one

Comment thread webpack/assets/javascripts/react_app/components/SearchBar/SearchBar.scss Outdated
@MariaAga
MariaAga force-pushed the move-css-foreman-js branch from 73c1f5a to f3c5055 Compare June 11, 2025 12:01
@evgeni

evgeni commented Jun 12, 2025

Copy link
Copy Markdown
Member

/packit build

@evgeni

evgeni commented Jun 12, 2025

Copy link
Copy Markdown
Member

Oh, I'm stupid. That still will fail, as there is no BuildRequires on that new package yet.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants