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.

Sign up for GitHub

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

Jump to bottom

CONSOLE-2361: Conversion of bootstrap table column and visibility classes to similar PF4 classes #9440

Merged
merged 1 commit into from Jul 14, 2021
Merged

CONSOLE-2361: Conversion of bootstrap table column and visibility classes to similar PF4 classes #9440

merged 1 commit into from Jul 14, 2021

Conversation

sg00dwin
Copy link
Member

@sg00dwin sg00dwin commented Jul 7, 2021

alerts

cluster-setttings

silences

explore-api

jobs

machine-autoscalers

machineconfigpools

volumes

@sg00dwin sg00dwin self-assigned this Jul 7, 2021
@openshift-ci openshift-ci bot requested review from kyoto and rhamilto July 7, 2021 22:32
@openshift-ci openshift-ci bot added the component/monitoring Related to monitoring label Jul 7, 2021
@sg00dwin sg00dwin assigned rhamilto and unassigned sg00dwin Jul 7, 2021
rhamilto
rhamilto reviewed Jul 8, 2021
View reviewed changes
Copy link
Member

@rhamilto rhamilto left a comment

Choose a reason for hiding this comment

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

Hooray for less Bootstrap classes!

frontend/public/components/cluster-settings/cluster-operator.tsx Outdated
'pf-m-hidden pf-m-visible-on-md',
'pf-m-hidden pf-m-visible-on-md',
'pf-u-w-16-on-xl',
'pf-u-w-33-on-2xl pf-m-hidden pf-m-visible-on-md',
Copy link
Member

Choose a reason for hiding this comment

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

It seems like in the other instances you are doing : hidden visible size. Would be good to be consistent.

frontend/public/components/job.tsx Outdated
'pf-m-hidden pf-m-visible-on-xl pf-u-w-16-on-xl',
'pf-u-w-25-on-2xl',
'pf-u-w-25-on-2xl',
'pf-u-w-25-on-2xl pf-m-hidden pf-m-visible-on-md',
Copy link
Member

Choose a reason for hiding this comment

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

It seems like in the other instances you are doing : hidden visible size. Would be good to be consistent.

frontend/public/components/machine-autoscaler.tsx Outdated
'pf-m-hidden pf-m-visible-on-lg pf-u-w-16-on-lg',
'',
'pf-u-w-25-on-xl pf-m-hidden pf-m-visible-on-sm',
'pf-u-w-10-on-2xl pf-u-w-16-on-xl pf-m-hidden pf-m-visible-on-lg',
Copy link
Member

Choose a reason for hiding this comment

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

It seems like in the other instances you are doing : hidden visible size. Would be good to be consistent.

Copy link
Member

Choose a reason for hiding this comment

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

Nit: extra space between pf-m-hidden and pf-m-visible-on-lg

frontend/public/components/machine-config-pool.tsx Outdated
'pf-u-w-33-on-md pf-u-w-25-on-lg',
'pf-u-w-25-on-lg pf-u-w-33-on-xl pf-m-hidden pf-m-visible-on-lg',
'pf-u-w-10-on-xl',
'pf-u-w-10-on-xl pf-m-hidden pf-m-visible-on-sm',
Copy link
Member

Choose a reason for hiding this comment

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

It seems like in the other instances you are doing : hidden visible size. Would be good to be consistent.

frontend/public/components/machine-config-pool.tsx
'pf-m-hidden pf-m-visible-on-md pf-u-w-16-on-md pf-u-w-8-on-lg',
'pf-m-hidden pf-m-visible-on-lg pf-u-w-8-on-lg',
'pf-u-w-25-on-lg pf-u-w-33-on-sm pf-u-w-8-on-lg',
'pf-u-w-33-on-md pf-u-w-25-on-lg',
Copy link
Member

Choose a reason for hiding this comment

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

It seems like other instances the sizes descend from largest to smallest. Would be good to be consistent.

Copy link
Member Author

@sg00dwin sg00dwin Jul 9, 2021 •
edited

Choose a reason for hiding this comment

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

I updated the sizes to consistent. Going from sm > md > lg > xl >2xl since PF rules apply to *-on-{size} and larger.
And pf-u-w-{width} rule precedes pf-u-w-{width}-on-{size}

frontend/public/components/monitoring/alerting.tsx
classNames('col-md-3', 'col-sm-4'),
classNames('col-md-2', 'hidden-sm'),
'pf-m-width-50 pf-u-w-33-on-sm',
'pf-m-hidden pf-m-visible-on-sm pf-m-hidden-on-md pf-m-visible-on-lg',
Copy link
Member

Choose a reason for hiding this comment

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

It seems like other instances the sizes descend from largest to smallest. Would be good to be consistent.

Copy link
Member Author

@sg00dwin sg00dwin Jul 9, 2021 •
edited

Choose a reason for hiding this comment

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

I updated the sizes to consistent. Going from sm > md > lg > xl >2xl since PF rules apply to *-on-{size} and larger. And pf-u-w-{width} rule precedes pf-u-w-{width}-on-{size}

@sg00dwin sg00dwin force-pushed the convert-table-classes-from-bootstrap-grid-column branch from 816c798 to c0d72ae Compare July 9, 2021 13:43
@rhamilto rhamilto changed the title Conversion of bootstrap table column and visibility classes to similar PF4 classes CONSOLE-2361: Conversion of bootstrap table column and visibility classes to similar PF4 classes Jul 9, 2021
rhamilto
rhamilto approved these changes Jul 9, 2021
View reviewed changes
Copy link
Member

@rhamilto rhamilto left a comment

Choose a reason for hiding this comment

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

/lgtm

/hold for approval

/assign @yapei

@yapei We just need a regression test to make sure there are no visual problems with these changes.

@openshift-ci openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Jul 9, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 9, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhamilto, sg00dwin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 9, 2021
@XiyunZhao
Copy link

/label qe-approved

@openshift-ci openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Jul 14, 2021
@XiyunZhao
Copy link

This PR has been verified on private build without any issue. below list the key checkpoint for this PR:

  1. When user adjust the browser size, the column of the table will follow the size automatically increase or decrease
  2. Only two mandatory column will shown on the table automatically when user use the minimize size of the browser
  3. The new update will not impact the content shown on the table, and the function of sort for the table
  4. No issue found on Roles, APIExplorer, ClusterOperator, Jobs, Machineautoscaler, Machineconfigpool, Alerting, Volumes page

@rhamilto
Copy link
Member

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 14, 2021
@spadgett
Copy link
Member

This PR has no user visible changes, and we have general docs and px approval for this epic. @ahardin-rh @sferich888 FYI

/label docs-approved
/label px-approved

@openshift-ci openshift-ci bot added docs-approved Signifies that Docs has signed off on this PR px-approved Signifies that Product Support has signed off on this PR labels Jul 14, 2021
@openshift-merge-robot openshift-merge-robot merged commit 9b50c3d into openshift:master Jul 14, 2021
@spadgett spadgett added this to the v4.9 milestone Aug 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rhamilto rhamilto rhamilto approved these changes

@kyoto kyoto Awaiting requested review from kyoto

Assignees

@rhamilto rhamilto

@yapei yapei

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/core Related to console core functionality component/monitoring Related to monitoring docs-approved Signifies that Docs has signed off on this PR lgtm Indicates that a PR is ready to be merged. px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR
Projects
None yet
Milestone
v4.9
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@sg00dwin @XiyunZhao @rhamilto @spadgett @yapei @openshift-merge-robot