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 #35287 - Create column selector on host index page #9323

Merged
merged 3 commits into from
Oct 30, 2022

Conversation

pkoprda
Copy link
Contributor

@pkoprda pkoprda commented Jul 26, 2022

User should be able to specify what columns wants to see on host index page. This approach using PF4 Selector hopefully can be applied for other index pages in the future.

Depends on #9351

@theforeman-bot
Copy link
Member

Issues: #35287

@ofedoren ofedoren self-assigned this Jul 26, 2022
@pkoprda pkoprda force-pushed the column-selector branch 2 times, most recently from 358405e to 6ef0743 Compare July 27, 2022 13:59
@pkoprda pkoprda marked this pull request as ready for review July 27, 2022 15:11
Copy link
Contributor

@adamruzicka adamruzicka left a comment

Choose a reason for hiding this comment

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

Unless I'm missing something, the filter button does nothing.

Maybe it is just my browser, but the elements on the page look a bit out of place. Do we have a ui mockup for this?

app/helpers/column_selector_helper.rb Outdated Show resolved Hide resolved
app/helpers/column_selector_helper.rb Outdated Show resolved Hide resolved
app/helpers/column_selector_helper.rb Outdated Show resolved Hide resolved
@pkoprda pkoprda force-pushed the column-selector branch 2 times, most recently from f6b3f69 to ec295ea Compare July 29, 2022 12:56
@pkoprda pkoprda marked this pull request as draft August 9, 2022 12:59
@pkoprda pkoprda force-pushed the column-selector branch 2 times, most recently from bb2e19f to 7fd3cb0 Compare August 11, 2022 08:37
app/helpers/selectable_columns_helper.rb Outdated Show resolved Hide resolved
app/helpers/selectable_columns_helper.rb Outdated Show resolved Hide resolved
app/helpers/selectable_columns_helper.rb Outdated Show resolved Hide resolved
app/helpers/application_helper.rb Show resolved Hide resolved
app/helpers/selectable_columns_helper.rb Outdated Show resolved Hide resolved
@ofedoren
Copy link
Member

Also, there is a conflict now :/

@pkoprda pkoprda force-pushed the column-selector branch 4 times, most recently from 8d4e5ea to 1f0d4d3 Compare September 14, 2022 15:26
@adamruzicka
Copy link
Contributor

When I try to save my selection, nothing happens and I get this in the browser console. What am I doing wrong?
image

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, @pkoprda, few suggestions inline for now, more to come :)

app/helpers/pagelets_helper.rb Outdated Show resolved Hide resolved
app/helpers/pagelets_helper.rb Outdated Show resolved Hide resolved
@pkoprda
Copy link
Contributor Author

pkoprda commented Oct 4, 2022

Adding some screenshots:
selectable-columns-screenshot-1
selectable-columns-screenshot-2


@MariSvirik any thoughts on this?

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.

There are still some problems:
123

1 - If there is another profile then checkboxes of other columns are selected even though they are not default ones.
2 - For some reason when I don't change selections via selector and reload the page, other columns appear (even though they are not default, see the gif)

app/helpers/pagelets_helper.rb Outdated Show resolved Hide resolved
app/helpers/pagelets_helper.rb Outdated Show resolved Hide resolved
app/registries/pagelets/manager.rb Outdated Show resolved Hide resolved
@MariSvirik
Copy link

MariSvirik commented Oct 5, 2022

@pkoprda I would just copy what we have in PF4. Can I see somewhere this implementation and click through it?

Table

  1. First of all, I would go with "Manage filters" copy instead of "Filter columns".
  2. The button should be closer to the end of the search bar: 16 px spacing between those two
  3. The button should be either a link button (without a box around it) as in the PF4 example or a secondary button.

Modal

  1. add a description: e.g. "Select columns to display in the table." Otherwise, I'm not sure what is happening there.
  2. The position of the modal should be fixed even when the rows collapse. Otherwise, it's super annoying.
  3. Based on your example I can clear out the entire table. We should have some default columns that you can't hide e.g. Name.

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.

I still encounter few issues:

  • codewise: names of the variables are not so consistent, slows the reading/reviewing. I've added few suggestions, but there is still room for improvements.
  • workwise: I guess after one hits Save the API call is asynchronous and it migth be the cause of the issue with old columns being selected after the page reloaded.

I'm going to continue with review tomorrow, I'm sending this just to save the notes :)

app/helpers/pagelets_helper.rb Outdated Show resolved Hide resolved
app/helpers/pagelets_helper.rb Outdated Show resolved Hide resolved
app/helpers/pagelets_helper.rb Outdated Show resolved Hide resolved
app/helpers/pagelets_helper.rb Outdated Show resolved Hide resolved
app/helpers/pagelets_helper.rb Outdated Show resolved Hide resolved
app/helpers/pagelets_helper.rb Outdated Show resolved Hide resolved
app/helpers/pagelets_helper.rb Outdated Show resolved Hide resolved
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, @pkoprda, there are few moments to fix, but otherwise it's almost done. Also, I like the new style more than previous button, so 🍪 for improving this as well :)

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, @pkoprda, I think now it's ready. I've added a final fix, nothing much. Let's finally get this in. Thanks for bearing with me this whole time!

@ofedoren ofedoren merged commit 6ba027c into theforeman:develop Oct 30, 2022
@pkoprda pkoprda deleted the column-selector branch October 31, 2022 13:06
@ekohl
Copy link
Member

ekohl commented Nov 23, 2022

@ofedoren please don't squash commits unless that's really what you want. We generally use the rebase option to merge.

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

Successfully merging this pull request may close these issues.

7 participants