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

Rule builder styling fixes. #5909

Merged
merged 6 commits into from
Apr 20, 2018

Conversation

jmchilton
Copy link
Member

See commits for details - mix of BS4 fallout fixes and enhancements (including some requested by @martenson in #5822).

@@ -338,7 +338,7 @@
<button @click="resetRulesAndState" :title="titleReset" class="creator-reset-btn btn rule-btn-reset">
{{ l("Reset") }}
</button>
<button @click="createCollection" :title="titleFinish" class="create-collection btn btn-primary rule-btn-okay" v-bind:class="{ disabled: !validInput }">
<button @click="createCollection" :title="titleFinish" class="create-collection btn btn-secondary btn-primary" v-bind:class="{ disabled: !validInput }">
Copy link
Member

Choose a reason for hiding this comment

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

having both btn-primary and btn-secondary classes on a button is superfluous I think

Copy link
Member

Choose a reason for hiding this comment

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

disregard, was amended in subsequent commit

@martenson
Copy link
Member

I am seeing really strange behavior when trying to define columns. The dropdown menu will intermittently not create a new row after click even though it goes through the 'active' state and closes. Sometimes it takes 4 attempts, other times it works on first, and even other times it just won't create the row ever (there is only so many times I can click). There is no js error.

screenshot 2018-04-18 11 47 02

@jmchilton
Copy link
Member Author

@martenson Not just you - I see it too. Hmm....

@jmchilton
Copy link
Member Author

That problem isn't this branch I don't think, it is dev.

@jmchilton
Copy link
Member Author

Looks like the fix is just to place the click handler on the outer li instead of the inner a target. Probably some spacing difference in Bootstrap 4 - makes sense that the tests wouldn't detect it since they literally click the a target.

@martenson
Copy link
Member

martenson commented Apr 18, 2018

yeah bs4 wants to have <div><a>... and not <ul><li><a>... but this is pretty horrible behavior, it looks like working but fails 'randomly'.

@jmchilton
Copy link
Member Author

Oh that probably explains the random really bright green in the downdown that is quite jarring also. I'll switch to use divs and a.

@martenson
Copy link
Member

@jmchilton no it does not, that is an actual change I made to increase contrast which is pretty bad by default in bs4. The color choice is weird on purpose, so I get feedback. ;)

@martenson
Copy link
Member

martenson commented Apr 19, 2018

I opened a PR against your branch: jmchilton#66 with some UX improvements (I hope).

The code has various help and title elements but often they do not seem to be reachable from the UI. I think we could improve accessibility of this component a lot if we consistently show titles on most/all of the active elements. For some critical places I would consider using a bs tooltip or even a popover.

MDN entry on <label> accessibility.

@martenson
Copy link
Member

In reoriented view the rules overflow into the input.

screenshot 2018-04-19 12 28 24

@jmchilton
Copy link
Member Author

Merged your PR into #5940 and disabled the reorient mode - I don't think that was working out. I actually use the wider view of the rules in #5926 when there is not preview available but I think if there is tabular data that mode is always just awkward.

I feel like all of the title and help elements I've added so far do show up in the GUI as mouseovers titles - but they could be more widespread and more prominent. The tooltips in the library GUI do look nicer than the browser mouseovers here. I'll see what what I can do.

@martenson martenson merged commit 13661ad into galaxyproject:dev Apr 20, 2018
@jmchilton
Copy link
Member Author

@martenson I've used bootstrap-vue to add many more tooltips and such to the rule builder in #5940, it made it very easy.

@martenson
Copy link
Member

@jmchilton marvelous!

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.

3 participants