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

Fixes #31320 - Remove old button css styling #9154

Merged
merged 2 commits into from Feb 15, 2021
Merged

Conversation

jturel
Copy link
Member

@jturel jturel commented Feb 9, 2021

This fixes the the whited-out styling on the user dropdown on the top right of the sync status page. It's a little scary removing a large chunk of css like this, but I'm almost positive it's not affecting other pages and that's what I'd like the reviewer to validate. I was able to confirm, at least, that the subclasses like actionlink and formbutton aren't used anymore.

@theforeman-bot
Copy link

Issues: #31320

Copy link
Member

@jeremylenz jeremylenz left a comment

Choose a reason for hiding this comment

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

Thanks for improving our final remaining ERB page in Katello, @jturel :)

Suggestion below.

@@ -163,78 +163,6 @@ input:focus {
@extend .status_exclamation_icon;
}

/* BUTTONS */
input[type='submit'], button, .button {
Copy link
Member

Choose a reason for hiding this comment

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

I searched in Chrome dev tools for the CSS from this file, and it's usually not even in the bundle. This entire file appears to only be used for this page (I suspect it would apply to any Katello ERB page, but Sync Status is the only remaining one as far as I know.)

With that said, there's probably even a lot more we could eliminate from this file. However, though this change fixes the problem it makes the "Synchronize Now" button look worse than it was before.

What are your thoughts on doing the following instead:

  1. Change this line to input[type='submit']#sync_button {. The button already has an id of "sync_button", so this will make the selector specific enough so that all of the following CSS only affects that button.
  2. Keep &:hover, &:active, and &:focus sections since they make the button interactive and visually pleasing as the pointer moves around.
  3. I agree that we can remove all the sections from &.dialogbutton down since they are classes that don't appear to be in use.
  4. In app/views/katello/sync_management/_products.html.erb, add classes to the button to make it look a bit better: change :class => 'button fr' to :class => 'btn btn-secondary button fr'

Copy link
Member Author

Choose a reason for hiding this comment

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

I interpreted the spirit of your feedback and just added btn btn-default to it, and I think it looks pretty good (consistent with pf pages). Let me know what you think

Copy link
Member

@jeremylenz jeremylenz left a comment

Choose a reason for hiding this comment

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

@jturel When the button is disabled, it's doing a weird thing where it gets a black border if you click on it (I think it's something with border when :active). But a similar behavior is also present in master, so I'll approve and leave it up to you whether you want to spend more time on it.

Other than that, works fine for me :)

@jturel
Copy link
Member Author

jturel commented Feb 15, 2021

@jturel When the button is disabled, it's doing a weird thing where it gets a black border if you click on it (I think it's something with border when :active). But a similar behavior is also present in master, so I'll approve and leave it up to you whether you want to spend more time on it.

Other than that, works fine for me :)

ACK. I'm gonna leave the button as-is!

@jturel jturel merged commit a09cb40 into Katello:master Feb 15, 2021
@jturel jturel deleted the button_css branch February 15, 2021 21:19
jjeffers pushed a commit that referenced this pull request Feb 17, 2021
* Fixes #31320 - Remove old button css styling

* Refs #31320 - use patternfly styling on Synchronize Now button
jjeffers pushed a commit to jjeffers/katello that referenced this pull request Feb 17, 2021
* Fixes #31320 - Remove old button css styling

* Refs #31320 - use patternfly styling on Synchronize Now button

(cherry picked from commit a09cb40)
jjeffers pushed a commit that referenced this pull request Feb 18, 2021
* Fixes #31320 - Remove old button css styling

* Refs #31320 - use patternfly styling on Synchronize Now button

(cherry picked from commit a09cb40)
jturel added a commit to jturel/katello that referenced this pull request Mar 4, 2021
* Fixes #31320 - Remove old button css styling

* Refs #31320 - use patternfly styling on Synchronize Now button

(cherry picked from commit a09cb40)
jturel added a commit that referenced this pull request Mar 10, 2021
* Fixes #31320 - Remove old button css styling

* Refs #31320 - use patternfly styling on Synchronize Now button

(cherry picked from commit a09cb40)
jturel added a commit to jturel/katello that referenced this pull request Apr 30, 2021
…us is whited out

Fixes #31320 - Remove old button css styling (Katello#9154)

* Fixes #31320 - Remove old button css styling

* Refs #31320 - use patternfly styling on Synchronize Now button

(cherry picked from commit a09cb40)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants