Fixes #21447 - The login page should be based on patternfly #4947
Conversation
|
Do not merge! This patch has not been tested yet. Can an existing organization member please verify this patch? |
1 similar comment
|
Do not merge! This patch has not been tested yet. Can an existing organization member please verify this patch? |
|
Issues: #21447 |
|
@sharvit |
|
@amirfefer Their implementation is different than their examples. |
|
[ok to test] |
sharvit
force-pushed
the
feature/patternfly-refactoring
branch
from
230da68
to
14f685a
Compare
|
Hi @Rohoover, Thanks. |
app/assets/stylesheets/login.scss
Outdated
| .login-page #brand { | ||
| position: relative; | ||
| top: -52px; | ||
| .login-pf { |
There was a problem hiding this comment.
These styles aren't provided in the patternfly css? Or is this another occurrence of us having to manually copy over styling because of the out of date patternfly-sass project?
There was a problem hiding this comment.
patternfly-sass is up to date in this case.
The styles here represent the differences between our design to patternfly design which is a bit different.
For example, patternfly using a background image, the brand positioning and size are different and some paddings inside the container are different.
| @@ -1,4 +1,5 @@ | |||
| <% content_for :title, _('Login') %> | |||
| <span id="badge"></span> | |||
There was a problem hiding this comment.
I don't see this id used in this PR, is this <span> necessary?
There was a problem hiding this comment.
@waldenraines
There is no use for span#badge as you mention unless you install the foreman_theme_satellite.
Then it should put the RedHat logo img inside the span#badge.
Removing the #badge cause this error because patternfly use it as a placeholder:
The old version used media queries in order to override this behavior and fix this issue.
I believe we should go with the patternfly implementation and keep it as a placeholder so we have less css code.
It means that I will need to update the foreman_theme_satellite project to inject the img inside the span#badge and not together with it before the #login like it does now:
|
@sharvit My old notes on this don't apply as it looks like things have changed. |
|
Patternfly created a new pr to fix this issue: I am going to update the login page to use the traditional alerts. |
|
@waldenraines, @Rohoover, @ohadlevy
|
sharvit
force-pushed
the
feature/patternfly-refactoring
branch
from
14f685a
to
9c03b94
Compare
| max-height: 17em; | ||
| overflow-y: auto; | ||
| .alert-danger { | ||
| @include alert-variant($alert-danger-bg, $alert-danger-border, $alert-danger-text); |
There was a problem hiding this comment.
Unexpected unknown at-rule "@include" at-rule-no-unknown
| .login-page .container .login { | ||
| padding-right: 40px; | ||
| .alert-warning { | ||
| @include alert-variant($alert-warning-bg, $alert-warning-border, $alert-warning-text); |
There was a problem hiding this comment.
Unexpected unknown at-rule "@include" at-rule-no-unknown
| padding-top: 0; | ||
| margin-top: 0; | ||
| .alert-info { | ||
| @include alert-variant($alert-info-bg, $alert-info-border, $alert-info-text); |
There was a problem hiding this comment.
Unexpected unknown at-rule "@include" at-rule-no-unknown
| // https://github.com/patternfly/patternfly/issues/804 | ||
| .login-pf .login-page .container { | ||
| .alert-success { | ||
| @include alert-variant($alert-success-bg, $alert-success-border, $alert-success-text); |
There was a problem hiding this comment.
Unexpected unknown at-rule "@include" at-rule-no-unknown
sharvit
force-pushed
the
feature/patternfly-refactoring
branch
from
9c03b94
to
421dfb8
Compare
|
I would stick with patternfly and the full size alerts. |
I honestly like the way the shorter alerts look but I think we should be consistent with patternfly recommendations and the rest of the application. |
|
Sounds reasonable. You can review and merge, no future commit will come. |
|
@waldenraines - please re-review. |
The
login.scssused to be a compiled version of the patternflylogin.scsshttps://github.com/patternfly/patternfly-sass/blob/master/assets/stylesheets/patternfly/_login.scss
This change is breaking the
foreman_theme_satelliteproject so we will need to update the relevant files there.Before the changes:
After the changes:
Notice that the alert styling changed to the way that patternfly designed them for the login page:
https://github.com/patternfly/patternfly-sass/blob/master/assets/stylesheets/patternfly/_login.scss#L52-L55