Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Various small bug fixes and workaround cleanup for mobiles #979

Merged
merged 9 commits into from Jul 29, 2015

Conversation

Projects
None yet
3 participants
Contributor

saivann commented Jul 26, 2015

Live preview: (Merged)

Commits should be self-explanatory and easier to review individually. I am submitting them together as some are related or depend on each other. I can however submit them separately if needed.

@crwatkins Can you check if the live preview above lets you read wallets' scores on iOS?

Contributor

crwatkins commented Jul 26, 2015

I can now expand (and hide) score descriptions on iPad. However, there continues to be the issue where two (or more) categories can be selected at the same time.

Contributor

saivann commented Jul 26, 2015

@crwatkins Thanks for testing! The issue you're describing was actually the intended behavior. Does that really create usability issues? If I design it to auto-shrink other score texts when a new one is selected, content moves on screen as a result, and this increases the risk of accidental click as well as making the demanded content appear in a different location than the one the user expects, which seemed a bit less intuitive to me.

Contributor

crwatkins commented Jul 26, 2015

Yes, the problem is the list of wallets. You can have multiple categories selected and you really don't know which category the displayed list of wallets applies to. There are also other strange interactions. Let's say you have both Desktop and Web selected, but you happen to know that you looking at the list for Web, but you really want to see the list for Desktop. You actually have to deselect Desktop by clicking it and then re-selecting it.

Also, it seems easier to select an item by mistake on this webpage than other webpages when you are attempting just to scroll, leading to more folks than usual in the above situation.

Contributor

saivann commented Jul 27, 2015

@crwatkins My understanding is that you don't consider this to be an issue with scores and the global menu of the website? The wallet menu is indeed different in that any parent menu entry triggers actions on the page, they're not just simple submenu containers...

I've added a commit and updated the live preview to apply the previous behavior only to that menu, as I have the impression that the new approach I've used is causing more issue than it resolves in that case, and would require a bunch of additional CSS to provide a better experience.

Does that look better to you?

Contributor

crwatkins commented Jul 27, 2015

Much better now. LGTM.

Contributor

saivann commented Jul 27, 2015

@crwatkins Thanks again for your guidance and testing!

In the absence of critical feedback, this pull request will be merged on July 29th.

Contributor

harding commented Jul 28, 2015

Reviewed the code (but did not test) and everything LGTM up to 77511cb. @saivann thanks for breaking this into separate commits---it made review much easier.

Contributor

saivann commented Jul 28, 2015

@harding Thanks for your review!

@saivann saivann referenced this pull request Jul 29, 2015

Closed

Beautify javascript #980

Contributor

saivann commented Jul 29, 2015

@harding I just added the commit that beautifies the code and updated the live preview. Thanks for offering yourself for a quick review, I was indeed unsure both about polluting that pull request, and submitting changes later and trigger javascript changes twice within a short period for no good reason.

Contributor

harding commented Jul 29, 2015

Commits up to d814977 LGTM. Thanks!

@saivann saivann merged commit d814977 into bitcoin-dot-org:master Jul 29, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@saivann saivann deleted the saivann:jsimprove branch Jul 29, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment