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

Custom asset field editor ux redesign #18577

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

cconard96
Copy link
Contributor

@cconard96 cconard96 commented Dec 18, 2024

Checklist before requesting a review

  • I have read the CONTRIBUTING document.
  • I have performed a self-review of my code.
  • I have added tests that prove my fix is effective or that my feature works.

Description

Fixes #18406

  • fields now include their label inside -> reduce width usage in the viewport
  • we remove the preview of the field nature (input/dropdown)
  • we keep preview of some attributes (mandatory, full width)
  • a side panel helps retrieve all fields and drop them into the viewport
  • a separate section permits the creation of custom fields
  • native and custom fields are clearly identified
  • A search input filters available fields
  • We have 2 states for a field in the viewport: default and selected
  • We show the controls for editing or hiding the fields only for selected state
  • grip for drag&drop is always shown (to be discussed, we may have the selected state also)

@cconard96 cconard96 self-assigned this Dec 18, 2024
@orthagh
Copy link
Contributor

orthagh commented Jan 6, 2025

Quick review after testing this early rework:

  • first of all: great job, it's much cleaner.
  • Applied attributes (full width, mandatory, etc) in the preview panel are not done for custom fields.
  • move the mandatory attribute after the label (like displayed in the final form), same for all other attribute icons like read-only
  • grip icons should have a grab pointer
  • Maybe, we can reduce a bit the contrast of the line between drag control and label
  • "New field" button should have a hover state
  • Search input doesn't work.
  • a suggestion (where I take opinion), maybe we should have at creation only a subset of native fields already placed in the preview panel. [Name, serial, manufacturer, Model] could be a good start. The idea behind this is to improve the discovery of how the right panel works. If all fields are directly in place, the right panel is empty and we don't understand clearly its purpose.

@orthagh
Copy link
Contributor

orthagh commented Jan 6, 2025

Quick addition:

image

  • Maybe we can make the fields a bit contrasted. I applied gray-100 and a transparent border to the fields.

@orthagh
Copy link
Contributor

orthagh commented Jan 6, 2025

While discussing drag controls, in fact, let's remove them completely:

  • add a grab cursor on the whole div of the fields
  • The discoverability of dragging (hover not available) in responsive view is not that important for admin features.
  • I think we still need a way to edit a field in responsive mode. Not sure about the best way. To avoid default/selected states (because of complexity), I suggest to keep hovering on the desktop view but always displaying controls on responsive.

@orthagh
Copy link
Contributor

orthagh commented Jan 6, 2025

Moar:

  • If any attribute is applied, like read-only, on my side, fields will always be displayed as full-width. Either this attribute is set or not. Moreover, if I remove all attributes, it keeps the full-width display.
  • For "New field" button, could you add btn btn-sm btn-ghost-secondary w-100 classes? The current hover style is not sufficient.

@cconard96
Copy link
Contributor Author

a suggestion (where I take opinion), maybe we should have at creation only a subset of native fields already placed in the preview panel. [Name, serial, manufacturer, Model] could be a good start. The idea behind this is to improve the discovery of how the right panel works. If all fields are directly in place, the right panel is empty and we don't understand clearly its purpose.

I think users will expect all of the native fields to be present already, especially if they are fields related to capacities they enabled. Also, if they are creating multiple custom asset types they may get annoyed if they have to keep enabling all of the native fields. This may be a place where a discovery tutorial could be useful, or just try to have an intuitive UI. I find the sidebar design intuitive as it is (maybe hide the section headers when they are empty), but I don't know how other people will see it.

@orthagh
Copy link
Contributor

orthagh commented Jan 7, 2025

  • A last bug, after saving and reloading, attributes are not applied on fields. A edit and save for a field temporary fix the issue

@orthagh
Copy link
Contributor

orthagh commented Jan 7, 2025

I think users will expect all of the native fields to be present already

ok.

maybe hide the section headers when they are empty

I would like to see how it renders

@orthagh
Copy link
Contributor

orthagh commented Jan 8, 2025

maybe hide the section headers when they are empty

Ok for the current state. I guess we should always keep "custom fields" section because the button for adding will always be present.

Apart this, PR is ok for me, please fix tests, and we will go for merge.

@cconard96 cconard96 force-pushed the enhance/custom_fields_ux branch from 3a8108a to 9ca2f86 Compare January 8, 2025 14:59
@cconard96 cconard96 marked this pull request as ready for review January 8, 2025 15:03
@cconard96
Copy link
Contributor Author

Remaining test failure doesn't seem related at all.

@cedric-anne cedric-anne self-requested a review January 14, 2025 14:18
@cedric-anne cedric-anne force-pushed the enhance/custom_fields_ux branch from bc84fcf to e3c373a Compare January 16, 2025 08:52
Copy link
Member

@cedric-anne cedric-anne left a comment

Choose a reason for hiding this comment

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

When the edit form of a custom field that has just been added is opened, its options/default values are not shown in the form. They become available once the main form is saved.

@AdrienClairembault
Copy link
Contributor

Here is my general feeling UI wise.
Note that some of that might of course be subjective and everyone might not agree :)

1) Missing borders
image

2) Miss-alignement
image

3) The margin here is too big
image

4) The separation between the main area and the side panel would be clearer with added contrast using a different background color like we do for tickets
image

5) Edit modal has no title
image

6) On the same modal, the primary action should be on the right to be consistent with the rest of GLPI's modal.
The cancel action should be either a btn-ghost-secondary button or a btn-white button to draw less attention.

Examples of ghost and white button from tabler demo for reference:
image
image

7) Save button icon should use tabler icon instead of font awesome, and probably needs a me-1 margin

8) I feel like every tab should have a clear title at the start of its content

image

9) Grey items on white background feels wrong. I don't have the UI designer knowledge to justify it, but I "feel" this is something that could be avoided when possible.
Maybe using shadows instead ?
image

How was it done on your reference image @orthagh ?

10) Maybe the add field button should be in the main panel, at the end of the fields ?

image

I say this because when I click a big "Add" button which has the shape of a field, I expect the field to be placed at its position, if that make sense ?
Here we click on the button on the right panel and then something is added on the main panel.

I'm thinking of it because I did something similar a few days ago for the tiles configuration:

image

11) There is no way to delete a custom field that is here:

image

You have to insert it, edit then delete permanently which is not very intuitive.

12) The drag and drop placeholder is bigger than the items in the list:
image

Copy link
Contributor

@AdrienClairembault AdrienClairembault left a comment

Choose a reason for hiding this comment

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

See previous comment.

@orthagh
Copy link
Contributor

orthagh commented Jan 16, 2025

I have a few things to say about the previous comment, but not the time right now, wait a couple of hours.

@orthagh
Copy link
Contributor

orthagh commented Jan 16, 2025

So,

  • 1, 2, 3, 5, 7, and 12 are valid, nothing more to add.

  • while agreeing on 6, I want to highlight more requirements:

    • Primary action should always be on the right. If in a list of horizontal actions
    • Please prefer ghost for "cancel" action (and any opposition of primary action). Plain secondary is reserved for routine actions
  • 8, I don't have a definitive preference on that, but let's test with "Customize fields"

  • 4 and somehow related 9, I prefer how it currently is. On this particular, it's personal preference, and I think the UI is ok.
    If it bothers you too much, I suggest we address this later, after the pr merged.

  • 10, I think no, because we need to make evidence that we are adding a custom field, in opposition to a native one).
    This was the main emphasis of the design, having a sidebar to let the user understand there are native and custom fields.

  • 11 agree, but I think we can live with it. The edit button here will be too much for this space. And most of the time the fields will be added in the viewport. So I think we can live with the current state

@AdrienClairembault
Copy link
Contributor

Regarding this:

11 agree, but I think we can live with it. The edit button here will be too much for this space. And most of the time the fields will be added in the viewport. So I think we can live with the current state

How about a trashcan button when hovering the field like its done for actions on fields in the main panel ?

Ok for the rest.

@cconard96
Copy link
Contributor Author

Point 3 and 7 aren't related to this PR. The gap here exists for all forms using the generic_show_form template (or any individually using the components/form/buttons.html.twig template at least. The save button added by components/form/buttons.html.twig uses the FontAwesome icon.

My opinion on 8 is it is fairly intuitive without a header. The user navigated to the Fields tab so it makes sense that you can do stuff with/customize the fields here. The Capacities and Translations tabs don't have a header either.

@cconard96
Copy link
Contributor Author

The other changes have been addressed. The placeholder sizing was exactly the same as the sortable elements previously. The visual discrepancy was because the field previews had margins so they weren't all directly next to each other. The sortable elements all are 50% or 100% of the container size depending on if the field is full width or not. I've tried adding a hack to make the placeholder look like the displayed content, and while I succeeded, it also made sorting the fields a lot less usable at least on my end.

@orthagh
Copy link
Contributor

orthagh commented Jan 17, 2025

1, We may add a bit more space on the right of the border (the separation between viewport and available fields), it's a bit tight right now (equalize with the space on the left)

3,7: I add this to backlog (#18744 & #18745)

8, ok for title absence, honestly I was not that convinced by the need on this particular page.

It also made sorting the fields a lot less usable at least on my end.

I didn't find any usability issues, do you have hints?

How about a trashcan button when hovering the field like its done for actions on fields in the main panel ?

@cconard96 could you test that?

@AdrienClairembault
Copy link
Contributor

Point 3 and 7 aren't related to this PR. The gap here exists for all forms using the generic_show_form templat

Maybe we shouldn't use the generic_show_form here then ?
It is made to display the form of an asset, which is not the case here.

@cconard96
Copy link
Contributor Author

Maybe we shouldn't use the generic_show_form here then ? It is made to display the form of an asset, which is not the case here.

It is mostly used for assets, but honestly it is used in other places even with overriding the entire fields block just because it is nice and easy to use as a starting point to get the same layout, buttons, and create/modification date sections.

@cconard96
Copy link
Contributor Author

I didn't find any usability issues, do you have hints?

Note the placeholder jumping between columns when I drag it straight up within a single column. It isn't something I noticed before the recent changes and I don't have ideas for how to fix it.
Peek 2025-01-17 06-54

@AdrienClairembault
Copy link
Contributor

it is nice and easy to use as a starting point to get the same layout, buttons, and create/modification date sections.

I understand that, but if it isn't compatible with the layout you want to achieve (margin issues) and that you don't use most of its benefits (no form fields and no creation/modification date) then it might be the wrong tool for the current task.

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.

UX improvements for custom fields editor
4 participants