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

Various Improvements for the Rule Builder Widget #5823

Merged
merged 9 commits into from
Apr 13, 2018

Conversation

jmchilton
Copy link
Member

@jmchilton jmchilton commented Apr 1, 2018

  • Ticks off four checkboxes from Rules Widget - Post Merge Smaller Tweaks #5822.
    • Adds a "Close" button to the upload widget - to bring it inline with other upload modalities. 595c762
    • Adds a "Reset" button to the upload widget - to help usability if something large and awkward gets pasted into the widget. 595c762
    • Improve button styling and wording throughout. d126353
    • Add an X next to column definition options in the column editor mode - these were previously only available in the preview mode which is nice but obviously if you are editing column definitions this should be an option. 109e97d
  • Replaces the JavaScript regular expression handling with Python regular expressions - this should make it possible to execute the same rules on the backend as the front end. I think Python regular expressions syntax for replacements is a bit more standard than the JavaScript alternative. Executing the rules on the backend is important for the use case in Allow rule based operations on existing collections. #5819 specifically but would be important to do this at large scales or to integrate the rule syntax more closely with the APIs for creating collections which is also on the collection roadmap. For long term reproducibility I think it is important this commit in particular makes it in before 18.05 is branches. fc8491b
  • Brings in smaller updates, bug fixes, and clean ups from Allow rule based operations on existing collections. #5819 aimed at making the rule builder more applicable to additional use cases. 2254dba, 8b3fe21, 47795ea

Review individuals commits for additional details - I've tried to keep them atomic and focused.

Previously these were only available in preview mode.
- Replace "Okay" with "Apply" on button labels.
- Create CSS style for buttons in rule editor part of the component.
- More consistency - right-align all the buttons, more consistent spacing.
- Make primary action that button blue to make the buttons look a little bland.
@jmchilton jmchilton changed the title Small Tweaks to Rule Builder Various Improvements for the Rule Builder Widget Apr 6, 2018
@mblue9
Copy link
Contributor

mblue9 commented Apr 8, 2018

Hi @jmchilton I had a couple of other small thoughts for the Rule Builder that I wondered if I should add here (not sure if you've already done something on these)...

  • could the compulsory Definitions e.g. List Identifier and Collection Name be bolded or highlighted in the description (see below) to be more noticeable (I initially missed that they were compulsory)

screen shot 2018-04-07 at 4 34 23 pm

  • what about using just "Identifier" instead of "List Identifier", as wondering if users might understand it better as an identifier column for sample/datasets than a "list"
  • could the compulsory Definitions e.g. List Identifier and Collection Name be prepopulated in the left panel, so that it's quicker for the user, then they don't need to "add Definition", they only need to select which column
  • could the URL and FTP Definitions be combined into a single Definition for e.g. source/filepath then that could be added as another prepopulated Definition

@jmchilton
Copy link
Member Author

could the compulsory Definitions e.g. List Identifier and Collection Name be bolded or highlighted in the description (see below) to be more noticeable

"Collection Name" is definitely not mandatory, that only needs to be set if multiple collections are being created at the same time. I'd expect typically one collection would be built at a time - though I understand control/condition situations are currently generally handled as separate collections so I may be wrong about this. There is some discussion of this here.

If a list identifier is not set the build button is not supposed to be clickable, this a bug and it will fix it. I will make it bold for sure though. In some scenarios this is pre-populated. Like if you select multiple datasets in the history panel and then go to create a collection from them - the list identifier is pre-populated as the name. For general tabular data I'm more skeptical about pre-picking a column but I should find a way to put it in the list in a way where it is more clear it is a decision the user MUST make in addition to just making it bold. Thanks for the suggestion!

could the URL and FTP Definitions be combined into a single Definition for e.g. source/filepath then that could be added as another prepopulated Definition

I'm hesitant to combine URLs and file paths into one column definition. I understand the Galaxy uploader works that way and most of the time it is fine - but the backend for that is terrible hacks all the way down and prevents you from doing things like uploading tables that start with URLs for instance. I'd like to at least start with this trying to be more structured.

I think a more conservative approach I'd rather start with is searching the supplied data and pre-populating a URL column definition if it looks like there is a single column with a single URL in it. Maybe I could customize that message in that case also - so it says "We have found a URL we are assuming contains the target data, click here or remove the column definition to undo this if this is incorrect." The widget also pre-assigns FTP directory if you select that option in the upload page - so we could do something similar in that situation. So you'd only ever see that message if you are uploading a tabular data file without an obvious target URL - maybe it has multiple URLs or maybe it is a sample sheet with FTP paths embedded somehow. These would be more niche, more advanced situations.

what about using just "Identifier" instead of "List Identifier", as wondering if users might understand it better as an identifier column for sample/datasets than a "list"

I see, that is confusing. Hmm... the problem is that the paired indicator also gets translated underneath to an identifier - so this is a particular kind of identifier. Let me think about this though because you are definitely correct this is confusing. There is some help text on mouse over of the column definition on the outer page that says "This should be a short description of the replicate, sample name, condition, etc... that describes each level of the list structure." But that is a very subtle thing.

You've clearly put a lot thought into this and I really appreciate all the comments. This is great feedback that I think is going to make the widget much more usable. Keep it coming!

@martenson martenson merged commit eb67d42 into galaxyproject:dev Apr 13, 2018
@martenson
Copy link
Member

Thanks @jmchilton.

@nsoranzo nsoranzo deleted the rule_tweaks branch May 16, 2018 15:05
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.

4 participants