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

Small side panel formatting fix: hide button <p> containers instead of buttons; fix prev/next frame animation buttons #514

Merged
merged 13 commits into from
Jul 30, 2021

Conversation

fedarko
Copy link
Collaborator

@fedarko fedarko commented May 1, 2021

This doesn't entirely fix the problems brought up in #513, but it is a start -- it removes some unnecessary whitespace in the sample/feature metadata coloring and animation panels.

Hiding the button(s) inside a <p> tag still leaves some extra whitespace, since empty <p> tags still take up some space. Hiding the <p> tag directly (instead of the button(s)) fixes this, so now for example the default state of the animation panel looks a bit nicer:

previous now
image image

This PR also adjusts the animation panel so that:

  • disabled buttons (prev frame / next frame, when on the first/last frame) are now given a special style to indicate they're disabled
  • the next frame button is correctly disabled when on the last frame. It was off by 1 before, apparently.
prev frame button disabled next frame button disabled
image image

@emperor-helper
Copy link

The following artifacts were built for this PR: empire-biplot.qzv, empire.qzv, empress-tree.qzv, just-fm.qzv, plain.qzv

@fedarko fedarko changed the title Small side panel formatting fix: hide button <p> containers instead of buttons Small side panel formatting fix: hide button <p> containers instead of buttons; fix prev/next frame animation buttons May 1, 2021
@ElDeveloper
Copy link
Member

@fedarko can you pull from upstream to see the mchelper build?

@ElDeveloper
Copy link
Member

I added/removed a suggestion so that triggered a new build.

Copy link
Member

@ElDeveloper ElDeveloper left a comment

Choose a reason for hiding this comment

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

All looks good other than one minor comment that if you are comfortable can be addressed here or in a separate PR.

@@ -501,6 +501,11 @@ p.side-header button:hover,
background-color: dimgray;
}

.control p button:disabled {
Copy link
Member

Choose a reason for hiding this comment

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

This is slightly unrelated to this PR but would you be open to changing the name of this class to be empress-control or something that namespaces the class a bit better? Seems to me that control is too generic and prone to a collision with other CSS files.

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 agree that this name should be made more specific -- although I vote we move that to another PR. Making that change will require changing a lot of lines (see below), and I'd like to follow that up with a decent amount of manual testing to make sure it doesn't break something (just find-and-replacing control will cause problems, so the changes will probably have to be done manually...)

$ grep -ri " control " empress/support_files/templates/*
empress/support_files/templates/empress-template.html:        <!-- show control panel button -->
empress/support_files/templates/empress-template.html:        <button id="show-ctrl" title="Show control panel" class="hidden unselectable-text">&#9699;</button>
empress/support_files/templates/side-panel.html:    <button id="hide-ctrl" title="Hide control panel"
empress/support_files/templates/side-panel.html:<div class="side-content control hidden" id="sample-metadata-coloring-div">
empress/support_files/templates/side-panel.html:<div class="side-content control hidden" id="feature-metadata-coloring-div">
empress/support_files/templates/side-panel.html:<div class="side-content control hidden">
empress/support_files/templates/side-panel.html:<div class="side-content control hidden" id="barplot-div">
empress/support_files/templates/side-panel.html:<div class="side-content control hidden" id="shear-div">
empress/support_files/templates/side-panel.html:<div class="side-content control hidden" id="animation-div">
empress/support_files/templates/side-panel.html:<div class="side-content control hidden" id="export-div">
empress/support_files/templates/side-panel.html:<div class="side-content control hidden">
empress/support_files/templates/side-panel.html:<div class="side-content control hidden">
$ grep -ri "\.control" empress/support_files/css/empress.css
.control {
.control p {
.control p label:first-of-type {
.control p button {
.control p button:hover {
.control p button:disabled {
.control p button:only-child {
.control div.animate-btns {
 * Also, the reason for display: block is because the .control p CSS

@ElDeveloper
Copy link
Member

Sounds fair. Thanks @fedarko !

@ElDeveloper ElDeveloper merged commit 8ccd57b into biocore:master Jul 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants