Skip to content
This repository has been archived by the owner on Jul 21, 2023. It is now read-only.

[GAPRINDASHVILI] Fix "<select> per page" styling in patternfly 3.25 #362

Merged
merged 1 commit into from
May 31, 2018
Merged

Conversation

himdel
Copy link
Contributor

@himdel himdel commented May 31, 2018

The css comes from patternfly/patternfly#910, wrapped in a .miq-v2v so that we can be sure this only affects v2v screeens.

Unfortunately dialogs get opened directly under body, so any such class needs to live on body itself.

(Fortunately there are no cross-controller ajax transitions in gaprindashvili.)

The css comes from patternfly/patternfly#910, wrapped in a `.miq-v2v` so that we can be sure this only affects v2v screeens.

Unfortunately dialogs get opened directly under body, so any such class needs to live on body.

(Fortunately there are no cross-controller ajax transitions in gaprindashvili.)
@himdel
Copy link
Contributor Author

himdel commented May 31, 2018

Fixes those "per page" in

screen shot 2018-05-30 at 10 24 41 am

and

screen shot 2018-05-30 at 11 13 05 am

in gaprindashvili.

(from @AllenBW 's ManageIQ/manageiq-ui-classic#3963 (comment) )

(The paging styling is the same in gaprindashvili and master, from ManageIQ/ui-components#103.)

@AllenBW
Copy link
Member

AllenBW commented May 31, 2018

Very snazzy !! ❤️

SS of da fix in action

screen shot 2018-05-31 at 8 32 08 am

screen shot 2018-05-31 at 8 31 55 am

Copy link
Member

@AllenBW AllenBW left a comment

Choose a reason for hiding this comment

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

🌮 💃

@AllenBW AllenBW requested a review from priley86 May 31, 2018 12:35
@AllenBW
Copy link
Member

AllenBW commented May 31, 2018

Butttt also... this might be the reason we should make a g brach for v2v as well... cuz its just needed for gaprindashvili release 🤔

@himdel
Copy link
Contributor Author

himdel commented May 31, 2018

There is one, that's why this PR is against that branch :)

@AllenBW
Copy link
Member

AllenBW commented May 31, 2018

😆 yes yes I feel friday already... 🤦‍♂️

@AllenBW
Copy link
Member

AllenBW commented May 31, 2018

Ok related... riffing off this thing said here

Keeping g and master the same till the release of 5.9.4... We ought to reconsider how the labels are being used... Everything is G/yes right now... but there are somethings that are G/only, we could repurpose existing labels... but that muddies the meaning, could get confusing...

I would like to propose just a simple gaprindashvili or gaprindashvili/only label that will indicate the pr is only for g, doesn't have to be backported or any other action

@priley86
Copy link
Member

priley86 commented May 31, 2018

thanks for explaining this... i am following this now after discussing it w/ @AllenBW. I see that this CSS is only needed for gaprindashvili branch. I'm OK w/ this if others are.. can anyone confirm what version of PF is inside the gaprindashvili branch? It would be nice to know I guess just so we have a handle on how far back some things can go...but I understand this is a one off...

i don't have a strong preference on the labeling (w/e works w/ the existing process)...

@AparnaKarve do you have any more thoughts?

@AparnaKarve
Copy link
Contributor

I would like to propose just a simple gaprindashvili or gaprindashvili/only label that will indicate the pr is only for g, doesn't have to be backported or any other action

@AllenBW If a PR is created off the ManageIQ:gaprindashvili branch (like this PR), then we don't need the gaprindashvili/yes label at all - since it's implicit that the PR is meant for Gaprindashvili only -- this is the current practice in the ManageIQ/manageiq-ui-classic and other ManageIQ repos, so let's stick to that.

Until the release, let's keep the gaprindashvili branch synched-up with the current master.
The gaprindashvili branch may have a few extra PRs (like this one) -- and that is OK.
But for the most part, until the release, gaprindashvili and master have identical behaviors.

The official gaprindashvili/yes and gaprindashvili/no labels would surface in the v2v repo post 5.9.4 release.
Post 5.9.4 release, is when we would be making decisions like - if a bug fix or feature needs to be backported to Gaprindashvili (5.9.5) or not.
Again, this is in-line with our current practice in the other repos.

@AllenBW
Copy link
Member

AllenBW commented May 31, 2018

Ahhh so @AparnaKarve the collection of prs with g labels that were already merged, should not have been assigned g labels cuz there is a bunch of em

@AparnaKarve
Copy link
Contributor

Thanks @AllenBW, I did not know about the gaprindashvili/backported label

gaprindashvili/backported has a special meaning and implies a cherry-pick backport on the gaprindashvili branch -- pretty sure we will need this label post 5.9.4, but just not now.

@AllenBW
Copy link
Member

AllenBW commented May 31, 2018

Cleaned it up, feels a lil less 😕 😌 🎊

@AparnaKarve
Copy link
Contributor

@AllenBW Thanks!
Also thanks for testing this PR -- merging it now.

@himdel Thanks for the PR

@AparnaKarve AparnaKarve merged commit d573562 into ManageIQ:gaprindashvili May 31, 2018
@himdel himdel deleted the pf910-perpage branch June 1, 2018 11:26
@himdel
Copy link
Contributor Author

himdel commented Jun 1, 2018

Thanks everybody :)

can anyone confirm what version of PF is inside the gaprindashvili branch?

@priley86 gaprindashvili ui-clasic is using patternfly 3.25.

As for the other PRs, only 2 of them are not really necessary for master:

The rest should still be good for both :)

@himdel
Copy link
Contributor Author

himdel commented Jun 1, 2018

Oh... but if it would help .. this PR can also go in master and not break anything.
I made it against gaprindashvili mostly because it's not needed in master and is not particularly pretty, but it won't break master if keeping the branches identical for now is a goal.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants