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.

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

Merged
merged 1 commit into from
Sep 21, 2023

Conversation

MariaAga
Copy link
Member

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

@theforeman-bot
Copy link
Member

Issues: #32748

Copy link
Member

@ofedoren ofedoren left a 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:

  1. 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.
  2. The bigger one: when a user with view only permissions visits a page, the bookmarks are not quite usable.

I've captured this in a gif:
123

@MariaAga
Copy link
Member Author

Thanks for the review!

  1. Changed the link behavior in the models page
  2. did 2 fixed: changed the search bar length to max 600px, changed the bookmarks to open on the left of the toggle. (the bookmarks were usable before this, but users had to scroll left to see the dropdown)

@ofedoren
Copy link
Member

ofedoren commented Jun 8, 2023

Thanks, @MariaAga, but it still looks odd:

  1. If a user with view only permissions goes to a page, the bookmarks are okay-ish? Still looks weird with all that space between the search and the bookmarks:
    ScreenShot-1686237946889
  2. If a user with all permissions goes to a page, the bookmarks are even more confusing by being at the middle of the bar:
    ScreenShot-1686237874561

Is that OK from UI/UX perspective?

@MariaAga
Copy link
Member Author

I can't reproduce this bug can you check if all the css in this pr is applied in your env?
added a fix so translatedInitialSortColumnName will be the first sorted column and not the first column.

@MariaAga MariaAga requested a review from ofedoren July 3, 2023 11:42
Copy link
Member

@ofedoren ofedoren left a 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:
ScreenShot-1690557905388

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,
Copy link
Member

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?

@ares
Copy link
Member

ares commented Aug 21, 2023

Code-wise LGTM, but I'd rather ask someone with better React/JS knowledge to re-check.

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?

@MariaAga
Copy link
Member Author

this pr is also still not ready as I need to address Olehs comments

@ofedoren
Copy link
Member

ofedoren commented Sep 18, 2023

Quick note: (re)moving pf-m-align-right class from toolbar items seems to fix that weird spacing:
ScreenShot-1695045943714

UPD: Could we also add bulk selection as on ACS (/alternate_content_sources) page from Katello? I think it should be possible to extract that logic from the plugin and insert here?

@theforeman theforeman deleted a comment from ekohl Sep 18, 2023
@MariaAga
Copy link
Member Author

We can add bulk selection but in the next PR.
In case the css issue is some other css overriding the pf-m-align-right value, I added a more specific id so it won't be easily overridden

@ofedoren
Copy link
Member

ofedoren commented Sep 19, 2023

Well, it still doesn't help :/
ScreenShot-1695123641189

Did you have Katello plugin enabled while you were testing this? I do, and I think there is some issue with class="pf-c-dropdown pf-m-align-right", specifically pf-m-align-right looks like:

    width: 100%;
    justify-content: flex-end;
    display: flex;

If I change width to auto, then the issue is resolved. Not sure how much impact is that on the other places, but at least it fixes the searchbar.
ScreenShot-1695123684994

Also, this PR seems to break styling on Katello pages.
Before:
ScreenShot-1695123954773
After:
ScreenShot-1695126805490

@MariaAga
Copy link
Member Author

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

@ofedoren
Copy link
Member

ofedoren commented Sep 19, 2023

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:
Peek 2023-09-19 17-08

Maybe a large spinner with Loading... text?..

@MariaAga
Copy link
Member Author

Added so when there are no results, and the api request is pending it will look like this:
Screenshot from 2023-09-20 14-05-27
Also there is a spinner next to the search input

ofedoren
ofedoren previously approved these changes Sep 20, 2023
Copy link
Member

@ofedoren ofedoren left a 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 :)

@MariaAga
Copy link
Member Author

Thanks, fixed now

Copy link
Member

@ofedoren ofedoren left a 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 🌹

@ofedoren ofedoren merged commit 2efcf47 into theforeman:develop Sep 21, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants