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

Make widget spinners horizontal #7579

Merged
merged 9 commits into from
Jun 1, 2018

Conversation

AaronVanGeffen
Copy link
Member

@AaronVanGeffen AaronVanGeffen commented May 28, 2018

The current spinner widgets are rather tiny, leading to unfortunate misclicks and generally making them a pain to use. I'm talking, of course, about widgets like:
screenshot_20180528_224245

We can improve them dramatically by making the buttons horizontal, thereby increasing the click areas:
screenshot_20180528_224215

It appears these buttons aren't properly vertically aligned yet, but it works as a proof of concept. If we stick with the triangles, the widgets would currently look like:
screenshot_20180528_224332

I'd like some input before I start converting all spinner widgets… Your thoughts?

@AaronVanGeffen AaronVanGeffen added discussion Some input from team members is wanted. user interface Has to do with user interfaces. labels May 28, 2018
@Broxzier
Copy link
Member

Another idea:
image

This makes it much less likely you accidentally click the wrong button.

@AaronVanGeffen
Copy link
Member Author

Hmm… I agree that would prevent more misclicks, but it would also require more horizontal movement when you do want to adjust the value… I'm not convinced yet, but I like the idea.

It would require the value paint code for each of the widgets to be adjusted for the horizontal (left) offset as well, but that's a small price to pay for usability improvement, of course.

@stavrossk
Copy link

I like @AaronVanGeffen 's idea, the buttons are much easier to click on when they are aligned horizontally! I think the up/down arrows in the first picture are too small.

@Broxzier
Copy link
Member

image
Having the buttons outside of the box looks even better in my opinion, and allows them to be bigger without increasing the height too much.

@Gymnasiast
Copy link
Member

Gymnasiast commented May 29, 2018

I prefer @Broxzier 's - and + order. It's also in line with how GTK+ does it:

I have no strong preference as to where to put the - and + buttons. I do think we should use - and + symbols, and that the minus should come first.

@IntelOrca
Copy link
Contributor

The + and - buttons only make sense for numerical spinners. I think the only non-numerical spinner we have is the change ride type one.

Interestingly the land tool has this style with the subtract and positive buttons being on opposite sides.

@Gymnasiast
Copy link
Member

I think the only non-numerical spinner we have is the change ride type one.

That one is a weird hybrid of a dropdown and a spinner. It should probably just be a dropdown.

@IntelOrca
Copy link
Contributor

@Gymnasiast It will require implementing scrolling for dropdowns to handle the large number of items there are.

@AaronVanGeffen
Copy link
Member Author

AaronVanGeffen commented May 29, 2018

It will require implementing scrolling for dropdowns to handle the large number of items there are.

It looks like it fits as-is:

screenshot_20180529_102711

If you prefer to keep the spinners around, we could use different captions for the ride type selection spinner, for now. Perhaps the horizontal scrollbar's?

Having the buttons outside of the box looks even better in my opinion, and allows them to be bigger without increasing the height too much.

Agreed. I would prefer them to be gapless though, like in the GTK+ example:

screenshot_20180529_104601

@IntelOrca
Copy link
Contributor

@AaronVanGeffen oh yes, the grid text dropdown must have been introduced after it got changed to a spinner. It should be changed back.

@AaronVanGeffen
Copy link
Member Author

Done. We also should really look into sorting the dropdown in question... It'll require some additional bookkeeping, but it's doable.

@pizza2004
Copy link
Contributor

I like the idea of the buttons being at either end personally. I think it gives the best idea of taking away and adding to the box, which makes it the most intuitively pleasing in my mind.

@Gymnasiast
Copy link
Member

Gymnasiast commented May 29, 2018

@Gymnasiast It will require implementing scrolling for dropdowns to handle the large number of items there are.

No, it won't. Dropdowns can handle 128 items, there are 91 ride types (including unused slots inbetween).

@AaronVanGeffen oh yes, the grid text dropdown must have been introduced after it got changed to a spinner. It should be changed back.

Change back? It originally was a spinner. Then wolfreak99 turned it into a spinner/dropdown hybrid. It should not be changed back into a spinner, it should be changed into a normal dropdown (which Aaron has done).

@AaronVanGeffen
Copy link
Member Author

Change back? It originally was a spinner. Then wolfreak99 turned it into a spinner/dropdown hybrid. It should not be changed back into a spinner, it should be changed into a normal dropdown.

That's how I took @IntelOrca's comment. See 17500ce. I'll probably split it off to another PR.

@@ -97,9 +97,7 @@ enum {
WIDX_TEST_LIGHT,
WIDX_OPEN_LIGHT,
WIDX_RIDE_TYPE,
WIDX_RIDE_TYPE_INCREASE,
WIDX_RIDE_TYPE_DECREASE,
WIDX_RIDE_TYPE_APPLY,
Copy link
Member

@Gymnasiast Gymnasiast May 29, 2018

Choose a reason for hiding this comment

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

Removing the Apply button does increase the chance of unintended stuff happening is the user misclicks the dropdown. And since changing ride type is quite a hack, the consequences can be very annoying. I would recommend putting it back.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, but wasn't the whole reason for it to be introduced the fact that you didn't have an overview of all ride types at the time? You mentioned non-standard widget types elsewhere in the comments… We don't wait for confirmation for any other dropdowns, either. I personally don't think we should put it back.

Copy link
Member

@Gymnasiast Gymnasiast May 29, 2018

Choose a reason for hiding this comment

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

I think this is a special case, though, given its consequences when one selects the wrong option.
But if I'm the only one with that opinion, I'm not going to oppose it.

@Gymnasiast
Copy link
Member

That's how I took @IntelOrca's comment. See 17500ce. I'll probably split it off to another PR.

No need for that, it needed changing anyway, might as well do that in a PR that tackles spinners. Unless you expect this PR to take a lot longer, in which case: be my guest.

@wolfreak99
Copy link
Contributor

I'll miss the idea of the spinners, it made it very useful for experimenting with ride types, especially on flat rides.
Sure, maybe if you only used it when changing a rollercoaster track to another, It would be useful to automatically select what you drop down to and the buttons would just be extra. But being able to select the ride type when on flat rides to experiment with how they react, having an option that lets you iterate to the next option reduces the need for mouse movement: You move left 40 pixels, click, move right 40 pixels, click. It therefore provides quicker changing and also more easier to do.

@Gymnasiast
Copy link
Member

@wolfreak99 The thing is that it is a very non-standard widget. I don't think your scenario will be very common, and for most cases having a recognisable widget that works as one would expect will save for more time.

@Broxzier
Copy link
Member

@IntelOrca's comment gave me an idea, but maybe it deserved its own PR. Scrolling while hovering over a spinner could increase and decrease its value, much like how the land and water tool change the tool size.

@AaronVanGeffen
Copy link
Member Author

AaronVanGeffen commented May 29, 2018

No need for that, it needed changing anyway, might as well do that in a PR that tackles spinners. Unless you expect this PR to take a lot longer, in which case: be my guest.

Thing is, there's still some ongoing discussion about the spinners' appearance, which this distracts from. I'd also like to tackle the list being ordered orthographically as well (depending on the language, of course), which would fit with such a PR.

Scrolling while hovering over a spinner could increase and decrease its value, much like how the land and water tool change the tool size.

I like the idea! Probably best in its own PR, yes — although it's likely to conflict with this, and perhaps #5993

@AaronVanGeffen
Copy link
Member Author

AaronVanGeffen commented May 29, 2018

The work on the ride type selection widget has been split off to its own PR, #7581. Let's get the focus for this PR back to the spinners. We need to settle on a style. As far as I can tell, the main contenders are:

  1. image

  2. screenshot_20180529_104601

  3. image

  4. screenshot_20180529_192728

Feel free to debate. I'm hoping we'll reach a consensus, but if not, I'd like to wrap up and change the spinners in accordance with whichever is the most popular one after a few days.

@qwertychouskie
Copy link
Contributor

qwertychouskie commented May 29, 2018

I like how the buttons are in the area in the 3rd one (looks more polished) though not sure about having the buttons on opposite sides. EDIT: Looks like there is now a fourth option which is basically what I described. I vote 4 :)

@janzev
Copy link

janzev commented May 29, 2018

I would prefer the fourth option. There the buttons have a "clickable" size and it looks quite similar to the original buttons. But I would suggest to change the position of "+" and "-" so that the "-" comes first.

@wolfreak99
Copy link
Contributor

I like the 4th. The 3rd is too much mouse movement, unless the size of the text portion was reduced down to maybe 30% the original size.

@AaronVanGeffen AaronVanGeffen added pending review changelog This issue/PR deserves a changelog entry. and removed work in progress changelog This issue/PR deserves a changelog entry. labels Jun 1, 2018
Copy link
Member

@Gymnasiast Gymnasiast left a comment

Choose a reason for hiding this comment

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

I tested a lot of spinners. Most of them work correctly.
It looks like you forgot to modify one:

2018-06-01 21-54-49

@AaronVanGeffen
Copy link
Member Author

@Gymnasiast Nice catch! I've pushed a fix.

Note to self: squash some commits before merging.

Copy link
Member

@Gymnasiast Gymnasiast left a comment

Choose a reason for hiding this comment

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

Getest en goed bevonden. ^^

Copy link
Contributor

@marijnvdwerf marijnvdwerf left a comment

Choose a reason for hiding this comment

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

Not a fan of calling it a 'spinner'. In present-day interaction design that sounds more like some progress indicator than a numeric input. Apple's HIG uses the term 'stepper', and Microsofts Fluent guidelines have no mention of a 'Spinner' either.

I'd be fine with something like 'numeric input' or 'numeric stepper', but 'spinner' is too vague for me.

@AaronVanGeffen
Copy link
Member Author

@marijnvdwerf That's a little surprising, considering we've been calling the widgets such for years. I'm not against renaming the lot to 'stepper', though. Calling them 'numeric stepper' doesn't quite cover it, though, as it's used for non-numeric (but enumerable) things like months as well.

@IntelOrca
Copy link
Contributor

@marijnvdwerf but that is what they are called:
https://en.wikipedia.org/wiki/Spinner_(computing)

Microsoft call them up down controls which is not as nice to say or write.

@Gymnasiast
Copy link
Member

Gymnasiast commented Jun 1, 2018

The only people who have to understand what a spinner is are people touching the code, not the general public. Just keep it as 'spinner'.

@AaronVanGeffen
Copy link
Member Author

I do see @marijnvdwerf 's point, though. The spinner terminology makes sense for the up/down widgets, but not for the widgets we've implemented now. From what I can tell, he's right that 'spinner' is a common term for the new ones.

It's a small change that I don't mind making. Should be relatively painless.

@marijnvdwerf
Copy link
Contributor

@IntelOrca That article has no sources. https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/number talks about the same control, but 'spinner' is never mentioned.

@AaronVanGeffen It seems rather odd to use a +/- control to select a month; wouldn't a dropdown be more suited for that?

@AaronVanGeffen
Copy link
Member Author

It seems rather odd to use a +/- control to select a month; wouldn't a dropdown be more suited for that?

I agree that it would, but I also think that's a change for another PR.

@IntelOrca
Copy link
Contributor

We can just vote for it. Thumbs up for Spinner, thumbs down for Stepper.

@qwertychouskie
Copy link
Contributor

Gotta have them [w/f]idget spinners :)

@AaronVanGeffen AaronVanGeffen merged commit 635eab3 into OpenRCT2:develop Jun 1, 2018
@AaronVanGeffen AaronVanGeffen deleted the widgets/spinners branch June 1, 2018 22:06
@Gymnasiast
Copy link
Member

This is ridiculous. ONE person is in favour of changing to 'stepper', many are against it, and you still changed it.

@AaronVanGeffen
Copy link
Member Author

I, for one, thought Ted was joking when he proposed a vote. The terminology change really isn't a big deal to me. It'd be equally farcical to revert it now. Can we please just let this go?

@Gymnasiast
Copy link
Member

Even before Ted proposed a vote (joking or not), it was Marijn in favour, Ted and I against. More people against than in favour.

@AaronVanGeffen
Copy link
Member Author

Evidently I thought the idea had merit, otherwise I wouldn't have changed it… At worst, that's a tie. The spinner analogy works for vertical widgets, not horizontal ones, I think.

I really didn't want to drag this PR on for a terminology change, though. If I have merged it too quickly, I apologise.

@OpenRCT2 OpenRCT2 locked as off-topic and limited conversation to collaborators Jun 2, 2018
@OpenRCT2 OpenRCT2 deleted a comment from wolfreak99 Jun 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
user interface Has to do with user interfaces.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet