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

feat(saved search): restore the ability to order the list #14519

Merged
merged 6 commits into from Apr 21, 2023

Conversation

Rom1-B
Copy link
Contributor

@Rom1-B Rom1-B commented Apr 13, 2023

In 9.5 it was possible to manually order the list of saved searches, which was not possible since 10.0.

Before:

image

After:

image

Q A
Bug fix? yes/no
New feature? yes/no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets !27549

src/SavedSearch.php Outdated Show resolved Hide resolved
src/SavedSearch.php Outdated Show resolved Hide resolved
cedric-anne
cedric-anne previously approved these changes Apr 14, 2023
@cedric-anne cedric-anne added this to the 10.0.8 milestone Apr 14, 2023
@orthagh
Copy link
Contributor

orthagh commented Apr 17, 2023

Some remarks i would like to be addressed before merging this (i agree with the miss of previous feature)

  • 50px for handle is very large. I don't see the requirement for that
  • There's a lot of borders added and it's ugly
  • i see table tags are involved to do the design, we should avoid this. adding a simple icon to the left of the label should not require a table.
  • I would like to have a toggle to move to edit mode, drag controls should not be displayed by default
  • there is some right to update savesearches, i guess we should not allow all users by default

@cedric-anne cedric-anne dismissed their stale review April 17, 2023 09:17

Waiting for answers to latest comments.

@cedric-anne cedric-anne self-requested a review April 17, 2023 12:45
@orthagh
Copy link
Contributor

orthagh commented Apr 17, 2023

tested again:

  • there's still a larger border than 10.0/bf
    image
  • new icons should be based on tabler icons library, please don't use anymore font-awesome (lib is present for plugins, we should not use it anymore in core)
  • note about the icon choice for drag/drop, you must use grip-vertical for re-ordering vertically

Specific remarks on the toggle as it feels odd with the current design (functionally it's ok)

  • icon for [pin] action use a different design and i think more compact (we don't have lot of space here)
  • we may move the new action next to [pin]
  • the choice of fa-sort is very unusual, i'm not 100% sure how i would have addressed myself the design, but i think i would have explore a more classic [edit mode] design with a classic pencil. But i'm not 100% sure it's not too much . Checking on gmail design, they display the control on desktop only when hovering a row. The feature (moving a mail) doesn't seem to exist in mobile view.

I know i originally suggestion the toggle, but the gmail way seems interesting, to at least, test it and I'm curious to see if it can be better.

Copy link
Contributor

@orthagh orthagh left a comment

Choose a reason for hiding this comment

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

good to merge

@cedric-anne cedric-anne merged commit ea41dca into glpi-project:10.0/bugfixes Apr 21, 2023
11 checks passed
@Rom1-B Rom1-B deleted the support_27549 branch April 21, 2023 13:41
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.

None yet

3 participants