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

Vueify Tool Form Data Selector #16578

Merged
merged 86 commits into from
Sep 18, 2023
Merged

Vueify Tool Form Data Selector #16578

merged 86 commits into from
Sep 18, 2023

Conversation

guerler
Copy link
Contributor

@guerler guerler commented Aug 20, 2023

This PR rewrites the data selector field for tool forms using Vue and TypeScript. It replaces the previous select2 fields with vue-multiselect. The overall appearance of the input field has only been minimally adjusted, however there are some changes:

Move File Browser to Button Group
The file dialog button was previously placed to the right of the select field and is now shown in the button group on the left. The folder icon has been replaced with a dots/ellipsis to indicate that more datasets can be selected.

image



Treat DCE's as separate source
Dataset Collection Elements are not merged into the hda or hdca options list of the server response anymore but are instead treated as separate source. Server response in basic.py has been modified to account for that. However when it comes to displaying, the dce's are like before in the corresponding hda/hdca select fields.

TODO's

  • Avoid flickering when switching between source types
  • Allow width resizing and word break in multiselect field
  • Augment dce handling to properly match batch modes
  • Add test cases, streamline source handling
  • Add handling for optional data fields if not already covered
  • Refine data matching when using file dialog

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

@guerler guerler added kind/enhancement area/UI-UX kind/refactoring cleanup or refactoring of existing code, no functional changes labels Aug 20, 2023
@guerler guerler added this to the 23.2 milestone Aug 20, 2023
@guerler guerler force-pushed the form_data branch 10 times, most recently from 7e84105 to c38501e Compare August 23, 2023 12:10
@guerler guerler marked this pull request as ready for review August 23, 2023 12:10
@jmchilton
Copy link
Member

The DCE idea is cool - this feels a lot more helpful and structured. Thanks for implementing that.

@guerler
Copy link
Contributor Author

guerler commented Aug 23, 2023

Thanks for looking into it. I was wondering if this mechanism is also necessary for the ldda's. I have implemented it for now because that is how it was designed, but it might make more sense if the file dialog just imports the library datasets upfront and returns regular dataset and collection ids, instead of leaving that task to the tools api, but that's probably a question for the future.

Copy link
Member

@mvdbeek mvdbeek left a comment

Choose a reason for hiding this comment

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

Dataset Collection Elements are not merged into or disguised as hda or hdca entries anymore but are instead treated as separate source.

This looks wrong on first glance, I think this would break establishing what kind of map over we're dealing with. Please don't merge this before I haven't reviewed this.

lib/galaxy/tools/parameters/basic.py Outdated Show resolved Hide resolved
@mvdbeek
Copy link
Member

mvdbeek commented Aug 29, 2023

dce's are displayed when attempting to re-run a tool which consumed dce's but since users cannot select other dce's, the entry is displayed in a dismissible alert box instead of a select field. Dismissing the dce allows users to select an alternative input from the hda or hdca select fields.

Users used to at least know if this was mapped a collection, and they can of course select another item (DCE or not) by dragging it in. I think this makes the rerun UX worse

@guerler
Copy link
Contributor Author

guerler commented Aug 29, 2023

dce's are displayed when attempting to re-run a tool which consumed dce's but since users cannot select other dce's, the entry is displayed in a dismissible alert box instead of a select field. Dismissing the dce allows users to select an alternative input from the hda or hdca select fields.

Users used to at least know if this was mapped a collection, and they can of course select another item (DCE or not) by dragging it in. I think this makes the rerun UX worse

You can still drag another item into it, but you cannot combine dce with items of other types, which doesn't seem like a common use case to be honest.

@mvdbeek
Copy link
Member

mvdbeek commented Sep 9, 2023

I don't think we should have dce in the interface. We used to just say Select: <element_identifier>. If you really want to knock it out of the park you could let users navigate to the parent collection and/or say <element_identifier>: Element of Dataset Collection or <element_identifier>: Dataset Collection Element or Selected Dataset Collection Element: <element_identifier>

Screenshot 2023-09-09 at 15 32 19

@guerler
Copy link
Contributor Author

guerler commented Sep 9, 2023

@mvdbeek decided to replace dce with as dataset and as dataset collection depending on what the dce represents. I think we should leave more sophisticated labeling of dce entries to follow-ups.

@mvdbeek mvdbeek dismissed their stale review September 12, 2023 12:18

Looks better now, but haven't reviewed yet

Copy link
Member

@mvdbeek mvdbeek left a comment

Choose a reason for hiding this comment

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

Is the styling of the select content intentional ? It doesn't seem to align with our other colors
Screenshot 2023-09-15 at 14 34 20

It looks fine, but I don't feel qualified to make a judgement about the event bus. My gut feeling is that it's usually not a good idea, but maybe this is a good use ?

client/src/components/Form/Elements/FormData/FormData.vue Outdated Show resolved Hide resolved
@guerler
Copy link
Contributor Author

guerler commented Sep 15, 2023

Yes, the styling is standard and unrelated to this PR. This input element reuses the regular form select field. Styling options should be adjusted there. Regarding the eventbus, the usage of the eventbus avoids side effects and is cleaner than directly setting the value. It is also a non-critical feature since it is only used to show/hide the spinner. I think it is important to introduce such a critical input element change like this one early in the release cycle.

@dannon
Copy link
Member

dannon commented Sep 18, 2023

Thanks for swapping out the eventBus and the other changes -- it's definitely better to use the standard interfaces/pattern here. This is working pretty nicely in my testing; let's go ahead and get it into dev!

@dannon dannon merged commit 0daa4aa into galaxyproject:dev Sep 18, 2023
@itisAliRH itisAliRH deleted the form_data branch October 18, 2023 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/UI-UX kind/enhancement kind/refactoring cleanup or refactoring of existing code, no functional changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants