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 19 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
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
3 participants