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

Visually identify that a field is a drop-down list (single choice) #491 #525

Merged
merged 5 commits into from
Jun 13, 2023

Conversation

fflorent
Copy link
Collaborator

@fflorent fflorent commented Jun 1, 2023

Fix #491, for single choice lists only.

Remaining TODO:

  • Ensure it works well when the user only has read-only rights (seems to work just like the reference field);
  • Deduplicate code when relevant (lots of copy-paste from ReferenceEditor ⇒ is this relevant to deduplicate?);
  • Remove the chevron icon when editing from the table view (it should only appear in the detail view);

Help welcome for these questions 🙏:

  • Does this enhancement deserve to write unit tests? I would say no, as it does not introduce new feature per se.
  • Is it relevant to deduplicate the code from the Reference? It is mostly copy/paste of styles
  • Currently the chevron icon is even shown in the Grid when triggering the editor. How would you do that? I am not sure how to manage to remove it:
    • That seems not being doable through CSS, as the editor does not descent from the GridView's HTML element;
    • That seems not being doable through JS, the editor instance is not aware of the view (DetailView, GridView, ...) from which the user has triggered it (and I would not think relevant storing the view in the editor);

Here is how it is rendered as of the current state of the PR:

record4.webm

@fflorent fflorent marked this pull request as ready for review June 1, 2023 15:36
@georgegevoian georgegevoian self-requested a review June 5, 2023 04:26
@fflorent fflorent force-pushed the issue-491 branch 2 times, most recently from f1333e5 to d04903a Compare June 6, 2023 07:35
@georgegevoian
Copy link
Contributor

Thanks for opening the PR @fflorent.

To answer your questions:

  1. I agree. If you really want to, you can update an existing browser test or two to check for the presence (or absence) of the dropdown icon, but since it's just a cosmetic change, I don't think it's that important to have a test for it.
  2. I reviewed your changes and didn't see much duplication, so I think it's fine as is.
  3. Both ChoiceEditor and ChoiceTextBox should have the field available to them, which itself has a viewSection. You can check what type of view you have by inspecting the parentKey of the view section; for example, a card view will have a value of "detail" for the parent key. (It's a bit arcane - sorry!)

@fflorent
Copy link
Collaborator Author

fflorent commented Jun 7, 2023

Thank you @georgegevoian for your hint! I could end up to remove the chevron in the grid view when in edition mode.

It seems the PR is not updated, despite having updated the source branch. I will take a look tomorrow to see what went wrong while updating the PR. (Edit : it took a little while, but the PR finally got synchronized 🤷)

Cheers!

@fflorent
Copy link
Collaborator Author

fflorent commented Jun 7, 2023

The PR got synchronized by itself! 🎉

Copy link
Contributor

@georgegevoian georgegevoian left a comment

Choose a reason for hiding this comment

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

Any thoughts on whether we should also show the icon in card list views? They are very similar to card views, so it might be a good idea for the sake of consistency.

app/client/widgets/ChoiceEditor.js Outdated Show resolved Hide resolved
@@ -85,6 +90,7 @@ export class ChoiceTextBox extends NTextBox {
cssRow(
dom.autoDispose(disabled),
dom.autoDispose(mixed),
cssChoiceEditIcon('Dropdown'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
cssChoiceEditIcon('Dropdown'),

The config DOM is what's shown in the right panel (with the editor for choice options), so we don't want to add the icon here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Still seeing this line in the latest revision.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Weird, I thought that I have pushed the change… I fix that!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I hope that's better now, thank you for your vigilance!

@fflorent
Copy link
Collaborator Author

Thanks @georgegevoian, I have taken into account your remarks! :)

Copy link
Contributor

@georgegevoian georgegevoian left a comment

Choose a reason for hiding this comment

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

Thanks @fflorent!

@georgegevoian georgegevoian merged commit 3d808f6 into gristlabs:main Jun 13, 2023
fflorent added a commit to incubateur-territoires/grist-core that referenced this pull request Jun 20, 2023
…istlabs#491 (gristlabs#525)

* Visually identify that a field is a drop-down list (single choice) gristlabs#491

---------

Co-authored-by: Florent <florent.fayolle@beta.gouv.fr>
@vviers vviers added the anct label Jul 11, 2023
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.

Visually identify that a field is a drop-down list
3 participants