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

Add focus indicator to File Details tabs view #9956

Merged
merged 1 commit into from
Jun 26, 2018

Conversation

kevgathuku
Copy link
Contributor

@kevgathuku kevgathuku commented Jun 21, 2018

This is a potential solution to #9944

It enables the outline by default so that the website is easily navigable via keyboard
Feedback appreciated 😄

@codecov
Copy link

codecov bot commented Jun 21, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@ab266a7). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #9956   +/-   ##
=========================================
  Coverage          ?   52.07%           
  Complexity        ?    25984           
=========================================
  Files             ?     1651           
  Lines             ?    95983           
  Branches          ?     1290           
=========================================
  Hits              ?    49984           
  Misses            ?    45999           
  Partials          ?        0

@kevgathuku
Copy link
Contributor Author

kevgathuku commented Jun 21, 2018

Some screenshots of the navigable UI after the fix:

Sharing tab:
screen shot 2018-06-21 at 21 54 53

File list view:
screen shot 2018-06-21 at 21 54 28

@kevgathuku kevgathuku requested a review from jancborchardt June 21, 2018 20:14
@jancborchardt
Copy link
Member

@kevgathuku I also thought about just restoring the outline, but it doesn’t look so nice. What we should do is simply also add the proper :focus/:active effect so it works the same as for :hover. :) Like for example we did with #9951 (Keyboard navigation accessibility)

@kevgathuku
Copy link
Contributor Author

Thanks for the tips @jancborchardt
I'll look into implementing a similar solution for this

@jancborchardt
Copy link
Member

@kevgathuku great, thanks! :) Keep in mind to do it step by step, small separate pull requests. :) For example, start with fixing only the headers in the sidebar as you mentioned in the original issue.

@kevgathuku
Copy link
Contributor Author

Thanks once again. I'll focus on that section for now @jancborchardt

For the file details view the current hover effect is to underline the tab. I'm not sure the same underlining effect for the focus state would work very well.
It might cause confusion between the focused and selected tabs.

screen shot 2018-06-22 at 00 01 53

For example, in this image, the Comments tab is the selected one / currently active.
The Versions tab is hovered / in focus.

Restoring the browser default outline might just work, at least for the tabs section.

What do you think of this?
In the meantime, I'm also exploring other ways to do it that might be less confusing.

@jancborchardt
Copy link
Member

@kevgathuku I would say it’s two different steps: 1) visualizing :hover exactly as we do :focus and 2) thinking about how we want to visualize it.

I would say this pull request is about 1). :) And the current way to visualize is fine, but we can of course talk about it – but after we fix the basic accessibility.

@jancborchardt
Copy link
Member

@kevgathuku in other words: What you show in the screenshot is great! 😃

@kevgathuku
Copy link
Contributor Author

Got it @jancborchardt
I'll focus on the first step with the current effect 👍

@rullzer rullzer added design Design, UI, UX, etc. 3. to review Waiting for reviews labels Jun 22, 2018
@rullzer rullzer added this to the Nextcloud 14 milestone Jun 22, 2018
@jancborchardt jancborchardt added 2. developing Work in progress feature: accessibility bug and removed 3. to review Waiting for reviews labels Jun 22, 2018
@kevgathuku
Copy link
Contributor Author

kevgathuku commented Jun 22, 2018

Hi @jancborchardt would you happen to know if there is some guide on how to set up a local dev environment, or a resource you can recommend? I'm using a Mac currently and I'm currently having trouble setting up the project for development.

I'm currently following the instructions at https://docs.nextcloud.com/server/12/admin_manual/installation/system_requirements.html#recommended-setup-for-running-nextcloud and https://docs.nextcloud.com/server/12/developer_manual/general/devenv.html
and I'm stuck with installing nextcloud on the apache server.
Also not sure if this is the right place to ask this kind of question? If it's not please, point me to the correct place 😄
Thanks

@jancborchardt
Copy link
Member

@kevgathuku yup! The simplest guide for setting up a dev environment on macOS is at https://github.com/jancborchardt/nextcloud-scripts/blob/master/Nextcloud%20macOS%20development%20environment.md – let me know if that works. :)

@kevgathuku
Copy link
Contributor Author

Works like a charm. Didn't know there was such a guide 😄
Thanks @jancborchardt 👍

@jancborchardt
Copy link
Member

Works like a charm. Didn't know there was such a guide

Yeah, I need to add it to the docs :) good to know that it works for you too – I only checked it with friends macOSes cause I’m on Ubuntu. ;)

@kevgathuku kevgathuku changed the title Restore default outline to enable visible focus Add focus indicator to File Details tabs view Jun 25, 2018
@kevgathuku
Copy link
Contributor Author

This is once again ready for review
Changes:

  1. Enable focus indicator for file details tab. Current result with Versions tab focused:

screen shot 2018-06-25 at 10 28 43

  1. Enable click indicator for the tabs i.e. when a tab is focused, clicking on Spacebar or Enter should select the tab
    cc @jancborchardt

@jancborchardt
Copy link
Member

@kevgathuku nice! :) cc @nextcloud/javascript for review too.

I just noticed that the underline is now grey whereas it used to be black, which is much more visible. We should change it back to that, using:

border-bottom: 1px solid var(--color-text-lighter);

Also, for better clickability the minimum size of the tab headers should be 44px, which can be achieved using:

.tabHeaders .tabHeader {
    padding: 12px;
}

Can you adjust that @kevgathuku? :) (Since you are member of the Nextcloud organization here on Github, you can also do branches in the server repository itself, then we can collaborate easily on branches. :) Don’t worry about breaking stuff, as the main branches are protected.)

@@ -780,19 +780,12 @@ kbd {
font-weight: 600;
border-bottom: 1px solid var(--color-border);
Copy link
Member

Choose a reason for hiding this comment

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

This line can be removed because the border-bottom for .selected is set in line 788 down below. :)

@kevgathuku
Copy link
Contributor Author

Thanks for the pointers @jancborchardt
In future I'll create all branches on the repo. I agree, it will make collaborating much easier 😄

Finally, I've made the requested changes

@MorrisJobke MorrisJobke requested a review from juliusknorr June 25, 2018 15:22
@MorrisJobke MorrisJobke requested a review from skjnldsv June 25, 2018 15:22
@MorrisJobke MorrisJobke added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jun 25, 2018
@MorrisJobke
Copy link
Member

cc @nextcloud/designers

Signed-off-by: Kevin Ndung'u <kevgathuku@gmail.com>
Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

This is super awesome @kevgathuku! :) Just wrote a comment on a file using keyboard only and it worked well. 🎉

cc @nextcloud/accessibility for review too :)

@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jun 25, 2018
@MorrisJobke MorrisJobke merged commit a17750d into nextcloud:master Jun 26, 2018
@kevgathuku kevgathuku deleted the enable-outline branch June 26, 2018 07:37
@kevgathuku
Copy link
Contributor Author

Finally got merged 🎉 Thanks for all the feedback @jancborchardt
Happy to contribute more 😄

@skjnldsv
Copy link
Member

skjnldsv commented Jun 26, 2018

Finally got merged

5 days is pretty quick for us 😆

@jancborchardt
Copy link
Member

@kevgathuku awesome work! :) It would also be great to have you at our Nextcloud Conference in Berlin late August: https://nextcloud.com/conf – we provide up to 80% travel/accomodation support for contributors! 🎉

@kevgathuku
Copy link
Contributor Author

Amazing! I'll check out the conference and apply.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish bug design Design, UI, UX, etc. feature: accessibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants