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

gui: fix accordion titles #3172

Closed
wants to merge 17 commits into from
Closed

Conversation

norgeous
Copy link
Contributor

@norgeous norgeous commented May 23, 2016

Purpose

attempt to address some visual styling and accessability issues with all accordion title bars.
essentially following on from #3139

  1. convert entire accordion title bars to a links, rather than clickable divs.
  2. changed layout mode from inline to float for accordion title bars for pixel perfect spacing.
  3. fixed bootstrap tooltip that shows folder id when hovering folder labels.
  4. underlines are now never present in accordion titles (previously present with :focus).
  5. accordion titles appear on a single line (not multiline) with ellipsis for longer titles.
  6. this device is now available in the keyboard tab order.

Screenshots

v0.13.4
old

This PR
new

Adjusted gap between folder icons and folder title to be the same as gap
between identicons and device name.
Putting the spans on a single line is necessary, as they are inline and
the whitespace affects layout.
fix 'language' and 'actions' dropdowns that do not outline when tabbing
through on keyboard.
<span class="fa hidden-xs fa-fw" ng-class="[folder.type == 'readonly' ? 'fa-lock' : 'fa-folder']"></span>
<span ng-show="folder.label.length == 0">{{folder.id}}</span>
<span tooltip data-original-title="{{folder.id}}" ng-show="folder.label.length != 0">{{folder.label}}</span>
<span class="fa hidden-xs fa-fw" ng-class="[folder.type == 'readonly' ? 'fa-lock' : 'fa-folder']"></span><span ng-show="folder.label.length == 0">{{folder.id}}</span><span tooltip data-original-title="{{folder.id}}" ng-show="folder.label.length != 0">{{folder.label}}</span>
Copy link
Member

Choose a reason for hiding this comment

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

This was over multiple lines to prevent long lines, which you seem to have reintroduced.

@AudriusButkevicius
Copy link
Member

What exactly does this do? Removes the requirement for whitespace?

@norgeous
Copy link
Contributor Author

The whitespace that I have removed causes the underline to be all messed up, because they are inline elements and the multiple whitespace(s), such as indenting and new lines condense to a single space. Personally I would also prefer to have them on separate lines for readabilty, but it's not an option if it is to remain inline.

I will add some screenshots in the next 10 to clarify the overall purpose.

@norgeous
Copy link
Contributor Author

I will have a look at changing it somehow so that we can have the newlines =]. Let me get back to you...

@calmh
Copy link
Member

calmh commented May 23, 2016

The underline on those header links is custom though, right?

@norgeous
Copy link
Contributor Author

Not custom, they show when :focus. The underline is present in the latest release (v0.13.2), although it looks slightly better.

Once 37816e3 is applied it looks even worse (as shown in top half of screenshot) with the underline stretching out past the left of the words.

To get this underline to show:

  1. keep hitting tab, until its selected
  2. click on a folder panel to expand it (but make sure you click the folder name text if using 13.2) then move the mouse away.

both methods work for me to show underline in firefox and chrome win10.

@norgeous norgeous closed this May 24, 2016
@norgeous norgeous reopened this May 24, 2016
@norgeous
Copy link
Contributor Author

ok, i'm looking and thinking, I believe I can see a few things wrong here. Tooltips are not working on folders for example. Let me think some more and I will add some more commits in the next few days.

@calmh
Copy link
Member

calmh commented May 24, 2016

👍

@norgeous norgeous changed the title gui: fix ugly spacing in accordion titles and fix tab highlighting gui: fix accordion titles May 24, 2016
@norgeous
Copy link
Contributor Author

i updated the description of the PR. I think this is now done and this works way better than the current release. I'm happy to take any questions or comments at this stage and make further adjustments...

.panel-heading {
position: relative;
overflow: hidden;
.modal-header .fa{
Copy link
Member

Choose a reason for hiding this comment

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

Missing space before the brace

@AudriusButkevicius
Copy link
Member

LGTM, yet someone probably needs to compile and look at it.

@calmh
Copy link
Member

calmh commented May 28, 2016

Spacing is inconsistent on Safari/Mac

screen shot 2016-05-28 at 06 21 46

@AudriusButkevicius
Copy link
Member

That's just ms but why do we need ID + name + tooltip with ID?

@calmh
Copy link
Member

calmh commented May 28, 2016

The ID is only in the tooltip. The default folder just has a label that includes the ID to differentiate from all other "Default Folder"s that may exist.

@AudriusButkevicius
Copy link
Member

Ah

@norgeous
Copy link
Contributor Author

will fix that spacing on remote devices shortly

@norgeous
Copy link
Contributor Author

norgeous commented May 28, 2016

Spacing is inconsistent on Safari/Mac

Thanks! yep, fixed. wasn't just Safari tho, but fixed now...

@calmh
Copy link
Member

calmh commented May 30, 2016

LGTM now

@calmh
Copy link
Member

calmh commented May 30, 2016

@st-review merge please

gui: Improve layout of accordion titles

@st-review
Copy link

👌 Merged as 46fa5a3. Thanks, @norgeous!

@st-review st-review closed this May 30, 2016
st-review pushed a commit that referenced this pull request May 30, 2016
@st-review st-review added the frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion label Jun 16, 2017
@syncthing syncthing locked and limited conversation to collaborators Jun 16, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants