Skip to content
This repository was archived by the owner on Nov 6, 2018. It is now read-only.

refactor(toggle): less race conditions and works in FF#48

Closed
ijsnow wants to merge 1 commit intomasterfrom
toggle-refactor
Closed

refactor(toggle): less race conditions and works in FF#48
ijsnow wants to merge 1 commit intomasterfrom
toggle-refactor

Conversation

@ijsnow
Copy link

@ijsnow ijsnow commented Sep 17, 2018

This branch refactors the toggle so that it works in more browsers. It is just a button(still accessible input) rather than forcing range inputs to look how you want using webkit(Chrome/Safari only).

This also removes some logic that just screams race conditions and is almost certainly the real cause of this issue: https://github.com/sourcegraph/sourcegraph/issues/12976. I've fixed bugs in Sourcegraph in the past that were the result of race conditions that were uncovered in Firefox but not Chrome.

We might not want to merge this in immediately. I think it would be nice for @francisschmaltz to give the CSS a quick pass over. I got it looking as close as possible to the original as possible but it might not be perfect to the better trained eye.

Another reason to wait on merging this is I don't know a use case in the web app or browser extension where the toggle is disabled so I don't know if I kept the same UX that was intended. I believe the UI looks the same for this though.

Reviewing/working more on this PR will be easier if #47 gets merged in.

@ijsnow
Copy link
Author

ijsnow commented Sep 17, 2018

I would also consider removing the logic for "optimistic updates" of the toggle. Since the state should be controlled by a higher parent component, having local state that sometimes overrides parent state is another potential source of race conditions. The optimistic updates should be handled all in one place.

Copy link

@francisschmaltz francisschmaltz left a comment

Choose a reason for hiding this comment

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

Let's look at this in more detail, later.

@@ -1,52 +1,53 @@
.toggle {

Choose a reason for hiding this comment

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

Denying until we can spend more time to go over design.

Copy link

@chrismwendt chrismwendt left a comment

Choose a reason for hiding this comment

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

Nice, I tried this on Firefox and it looks good!

2018-09-17 12 44 45

No big deal, but on Chrome, the cursor is no longer a pointer when hovering over the knob.

Did this remove the slide-to-toggle functionality? I'm not attached to it, but it would be helpful to state that in the description if it was intended.

@ijsnow
Copy link
Author

ijsnow commented Sep 17, 2018

No big deal, but on Chrome, the cursor is no longer a pointer when hovering over the knob

Whoops, can easily fix with cursor: pointer;

Did this remove the slide-to-toggle functionality? I'm not attached to it, but it would be helpful to state that in the description if it was intended

It may have but it should still work since it's a button and only has two states so the on click should fire it. It's also important to add that sliding isn't really a thing I'd expect while using a website on a desktop(non-touch screen). Especially considering how small it is. I think that functionality is built in to range inputs for inputs with more than two states.

If it were a slider and not a toggle I would have reached for the range input.

@sqs sqs removed their request for review September 17, 2018 20:07
@sqs
Copy link
Member

sqs commented Sep 17, 2018

Removed myself as a reviewer - I trust y’all

@ijsnow ijsnow mentioned this pull request Sep 17, 2018
@chrismwendt
Copy link

@ijsnow I cherry-picked this into #56

@chrismwendt chrismwendt closed this Oct 5, 2018
@chrismwendt chrismwendt deleted the toggle-refactor branch October 5, 2018 02:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants