-
Notifications
You must be signed in to change notification settings - Fork 993
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 #32748 - add pf4 table template #9705
Conversation
Issues: #32748 |
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.
Thanks, @MariaAga, can confirm this change doesn't break webhooks plugin.
Although, I've noticed two issues:
- The small one: even when a user doesn't have permissions to edit, the link to edit page is still clickable. Not a big deal, but kinda misleading since it doesn't do anything. Although, if I open the link in a new tab, I get redirected to
no permissions
page. - The bigger one: when a user with view only permissions visits a page, the bookmarks are not quite usable.
Thanks for the review!
|
Thanks, @MariaAga, but it still looks odd:
Is that OK from UI/UX perspective? |
I can't reproduce this bug can you check if all the css in this pr is applied in your env? |
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.
Thanks, @MariaAga,, but I still see the same issue:
I've tried rm -rf node_modules && rake assets:clobber && rake assets:clean && npm install
, but it didn't help :/
Also, if the API request for fetching takes some time, the table is being shown as empty. If it takes more than 1-2 seconds, it gets a bit confusing. I see the small spinner instead of create
button, but can we make the table as a spinner if there is no data to show yet?
Code-wise LGTM, but I'd rather ask someone with better React/JS knowledge to re-check.
export const Table = ({ | ||
columns, | ||
errorMessage, | ||
getActions, |
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.
I don't see this to come from TableIndexPage, nor being documented, how can it be used/what for?
is there someone specific on your mind @ofedoren? if not, I'd recommend just getting this in, if you feel comfortable with the code. This looks like a big PR, perhaps a good time is right after the branching? |
this pr is also still not ready as I need to address Olehs comments |
a588eef
to
433e8c9
Compare
We can add bulk selection but in the next PR. |
433e8c9
to
726c2c4
Compare
726c2c4
to
93ac77a
Compare
Thanks @ofedoren, Katello has some css that overrides all tables css, I added an override in foreman, and will add a pr in katello so that their css for table will only affect katello tables |
Thanks a lot, @MariaAga. The last thing I'd like to ask for, could we make more obvious that the data is still loading? I mean, the empty state is confusing if the data are still being fetched: Maybe a large spinner with |
93ac77a
to
c8da6a8
Compare
c8da6a8
to
8ac82c9
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.
Small detail that can be fixed though: I've noticed that changing pagination will add JS style variable into the query:
https://foreman.kutak.com/models?per_page=5&page=1&search=name++%21%3D+%22%22&perPage=20
. Note both per_page
and perPage
.
Apart from that, ACK from me :)
8ac82c9
to
9ed451c
Compare
Thanks, fixed now |
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.
Thanks, @MariaAga, great work! And thank you for bearing with me for so long 🌹
Adds a template to create a table in pf4, usage example at:
webpack/assets/javascripts/react_app/routes/Models/ModelsPage/index.js
TableIndexPage still works as it did before if children are set (like in webhooks plugin)
Deletes all modals page files that are not needed with the template use
We should later add more buttons to the actions except for delete