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

Fixes #1489 - Adds keyboard navigation to the label editor. #1541

Merged
merged 1 commit into from
Jul 20, 2017

Conversation

brizental
Copy link
Contributor

Still need to add the functionality for when the user clicks Enter inside the text input and need to do some testing in other browsers, so far, I have only tested in Firefox.

Will do that in the next couple of days.

Anyways, r @zoepage?

@zoepage
Copy link
Member

zoepage commented Apr 30, 2017

@brizental Thanks for the PR! :)
I'm having issues with my setup atm. Will review as soon as it's fixed. Sorry! :(
Feel free to add the Enter feature when you have time.

Thanks!

@zoepage zoepage self-requested a review May 2, 2017 19:30
@zoepage
Copy link
Member

zoepage commented May 3, 2017

@brizental Fixed my setup and everything looks great :)))
Happy to review the Enter feature and merge!

@brizental
Copy link
Contributor Author

@zoepage Nice, thanks!
The Enter feature I have already finished, but everything is behaving differently in Chrome :/
I'll send in a commit for that, but I'll have to work on Chrome before merging. What do you think?

@brizental
Copy link
Contributor Author

@zoepage Now it seems like everything is working and ready to merge. Let me know if there is anything to change.

Copy link
Member

@zoepage zoepage left a comment

Choose a reason for hiding this comment

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

I checked Safari and it looks like it has issues with tabindex (http://caniuse.com/#feat=tabindex-attr). So the labels don't get selected.

label_safari
(Sorry for not thinking about checking that feature earlier in Safari.)

Chrome and Firefox have the border highlight, which is cool. But looks a little bit weird as the first label gets a border-bottom, but the following bottom and top. I'd suggest to either highlight the bottom, if it is easier or the whole box?

label_chrome

There is also the issue with the "gear icon" as it disappears from time to time after saving & closing. If you can reproduce, cool. If not, I'll open a new issue for it.

Thanks :)

@brizental
Copy link
Contributor Author

brizental commented May 12, 2017

@zoepage thanks for reviewing!

I'll check safari today then :)

@brizental
Copy link
Contributor Author

@zoepage sorry for the delay!

I fixed the border problem and the Safari problem.

About the border problem, I had to switch from outline to border, but border increases the size of the element a bit. This created a kind of "pop" that I thought looked nice, let me know if you also think it's cool.

About Safari, apparently tabindex does work on Safari, but for some elements it only works if you press opt+tab. Anyways, I just made a workaround so that it always works.

Finally, this problem with the gear icon disappearing is not happening for me... I think it would be best to open a new issue.

@miketaylr
Copy link
Member

what's the status of this PR? (I see it has some merge conflicts that need to be fixed, but beyond that?

@brizental brizental force-pushed the issues/1489/1 branch 3 times, most recently from 65cac48 to dbb430b Compare June 29, 2017 23:53
@brizental
Copy link
Contributor Author

So, me and @magsout worked at the All-Hands to rebase and make this pull request ready to merge. I just tested everything again just to make sure and everything seems right. Let me know if there are any other changes still @zoepage :)

@zoepage
Copy link
Member

zoepage commented Jul 10, 2017

Sorry it took me so long :/

So, it looks like the tab feature is not working in Safari(10.1.1) (probably again the same issue we had in the beginning?) and the Enter feature is not working in Nightly (56.0.0a1).

Tab feature:
tab_safari

The tab is not jumping into the list.

Enter feature:
enter_nightly

I can tab though the list, but when hitting Enter nothing happens.

--- edit ---
It also does not work in FF54.

@zoepage
Copy link
Member

zoepage commented Jul 19, 2017

As we spoke about it, it looks like I had caching issues. Sorry!

I'd say, we have one tiny thing left and then we can ship! 🎉

As you can see below, the outline works, but the focus behaves differently in all 3 browsers. FF has none, Chrome does, but it disappears when checking off the checkbox. Safari keeps it all the time.
Suggestion here would be to remove the focus. What do you think?

label_ff
label_chrome
label_safari

@brizental
Copy link
Contributor Author

Cool! I'm glad everything is working now :)

About that focus issue, that's weird. I was having that problem, but I added this line https://github.com/webcompat/webcompat.com/pull/1541/files#diff-d4646a9030e33cda244b11a534c258b5R132 to fix it.

I'll try and see if I can reproduce. For me everything in Chrome, Firefox and Safari are looking the same right now and focus is not showing in any of them.

@zoepage
Copy link
Member

zoepage commented Jul 19, 2017

Hm... That's weird. LMK if you can reproduce... Otherwise I'll check again.

@brizental
Copy link
Contributor Author

I tried @zoepage, but really couldn't reproduce. Maybe this is still some cache problem?
I even tried in different computers and nothing. Let me know :)

@miketaylr
Copy link
Member

Maybe this is still some cache problem?

@zoepage if you re-run grunt does the problem still happen?

@zoepage
Copy link
Member

zoepage commented Jul 20, 2017

The grunt task re-runs automatically, when we start the application. So, this was okay.
But I've cleared all caches and now it works. :)

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