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
tablecolumnclass updates public plugin PR 4 #9234
tablecolumnclass updates public plugin PR 4 #9234
Conversation
b3271e7
to
7412c09
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abhinandan13jan The PF classes that are added works well with All Projects
being selected in the project dropdown, which will add Namespace
column as the second row. when you select a project then Namespace
column will be hidden, then the classes doesn't work in some pages.
'pf-m-hidden pf-m-visible-on-lg pf-u-w-8-on-lg', | ||
'pf-m-hidden pf-m-visible-on-md pf-u-w-8-on-lg', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
classNames('col-lg-1', 'hidden-sm', 'hidden-xs', 'pf-u-w-10-on-lg'), | ||
classNames('col-lg-1', 'col-sm-2', 'col-xs-3', 'pf-u-w-10-on-lg'), | ||
classNames('pf-u-w-25-on-lg pf-u-w-33-on-md pf-u-w-50-on-sm'), | ||
classNames('pf-m-hidden pf-m-visible-on-xl pf-u-w-40-on-xl'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could also remove classNames
from this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
classNames('col-md-7', 'col-xs-6'), | ||
Kebab.columnClass, | ||
]; | ||
const tableColumnClasses = ['pf-u-w-42-on-lg', 'pf-u-w-58-on-lg', Kebab.columnClass]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
7412c09
to
359bf4d
Compare
'pf-m-hidden pf-m-visible-on-lg pf-u-w-16-on-lg', | ||
'pf-m-hidden pf-m-visible-on-md pf-u-w-16-on-md', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These classnames doesn't map with the existing classnames
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0967712
to
21f789b
Compare
Thanks @abhinandan13jan for updating the classnames, looks good to me |
/retest |
'pf-u-w-50-on-sm pf-u-w-33-on-md pf-u-w-25-on-lg', | ||
'pf-u-w-25-on-md', | ||
'pf-m-hidden pf-m-visible-on-lg pf-u-w-25-on-lg', | ||
'pf-m-hidden pf-m-visible-on-md pf-u-w-25-on-md', | ||
`${Kebab.columnClass} pull-right`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abhinandan13jan I think pull-right is an indicator that we waste space for the kebab/action icon column. I like that the most other changes has at least one column without a width, which takes over "all leftover" space.
Service accounts on a big screen before:
With this PR on a big screen (wastes space on the right side):
With this PR on a small screen (you can see the 25% space for the action column again and the date breaks without a "visible reason"):
I would suggest a css without styling for the name and without pull-right
.
'pf-u-w-50-on-sm pf-u-w-33-on-md pf-u-w-25-on-lg', | |
'pf-u-w-25-on-md', | |
'pf-m-hidden pf-m-visible-on-lg pf-u-w-25-on-lg', | |
'pf-m-hidden pf-m-visible-on-md pf-u-w-25-on-md', | |
`${Kebab.columnClass} pull-right`, | |
'', | |
'', | |
'pf-m-hidden pf-m-visible-on-lg pf-u-w-25-on-lg', | |
'pf-m-hidden pf-m-visible-on-md pf-u-w-25-on-md', | |
Kebab.columnClass, |
It looks like this then (yes, also not similar to the original design) on a big screen:
And like this on a small screen:
Wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
'pf-u-w-42-on-lg', | ||
'pf-u-w-58-on-lg', | ||
`${Kebab.columnClass} pull-right`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar here. pull-right
is only required because the namespace column is not shown when a namespace is selected. And the other one has a fix width. So the kebab column will take over the rest of the space.
This means on a ~ 1100px browser window, most space is wasted for the action menu and we have less then 300px for the name column. Which is many cases more then enough. But I personally would prefer an automatic width calculation from the browser instead of a hardcoded 5:7 (name to namespace) column width, which makes problem when the namespace is not shown.
'pf-u-w-42-on-lg', | |
'pf-u-w-58-on-lg', | |
`${Kebab.columnClass} pull-right`, | |
'', | |
'', | |
Kebab.columnClass, |
Wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
21f789b
to
22ac0fc
Compare
22ac0fc
to
bcb507f
Compare
Re-adding lgtm from @karthikjeeyar since the latest changes to fixes my comments above. Latest code change in service account and resource quota was small. Thanks for fixing the service account and resource quota |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhinandan13jan, jerolimov, karthikjeeyar 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 |
Addresses
https://issues.redhat.com/browse/ODC-5816
Screenshot
Test
No changes
Browser conformance
Chrome