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

Deck Options refactoring #1207

Merged
merged 63 commits into from Jun 21, 2021
Merged

Deck Options refactoring #1207

merged 63 commits into from Jun 21, 2021

Conversation

hgiesel
Copy link
Contributor

@hgiesel hgiesel commented May 29, 2021

This is a WIP about refactoring the deckoptions.

Some points which should be improved by this:

  • (?) Should look better on wider viewports
  • Have components dynamically adapt to wider viewports
  • Extension API

Some points of interest:

  • I rewrote the layout using Bootstrap Grid system (which uses CSS Flexbox, not CSS Grid, despite its name) with its .container, .row, and .col classes.
  • I factored out HelpPopup, the label handling, and RevertButton from the individual components. Making it obvious, that these are individual components, and can stand on their own. Of course this creates a lot of verbose code like this:
<Row>
    <Col size={7}>
        {tr.schedulingMaximumInterval()}
        <HelpPopup html={marked(tr.deckConfigMaximumIntervalTooltip())} />
    </Col>
    <Col size={5}>
        <SpinBox
            min={1}
            max={365 * 100}
            bind:value={$config.maximumReviewInterval}
        />
        <RevertButton
            defaultValue={defaults.maximumReviewInterval}
            bind:value={$config.maximumReviewInterval}
        />
    </Col>
</Row>

I could wrap these up into their own respective components, let's say SpinBoxRow, CheckBoxRow, etc., as to avoid boilerplate.

@hgiesel hgiesel marked this pull request as draft May 29, 2021 00:33
@dae
Copy link
Member

dae commented May 29, 2021

Re "look better", I'm not sure that's the case at the moment? Compared to what we had before, the label and the input box are spaced further apart, making it harder to see which label corresponds to which input box. Maybe instead of a 1fr 1fr layout after a certain size, it would look better as 1fr 2fr or similar? The maximum width is probably too large as well.

Screen Shot 2021-05-29 at 10 54 02 am

Screen Shot 2021-05-29 at 10 53 35 am

I think we'll want components like SpinBoxRow as well, as it's both easier to add new options that way, and will ensure consistency.

@dae
Copy link
Member

dae commented May 29, 2021

I also feel like the large text boxes feel a bit ugly - what I was going for with the original design was boxes that aren't too much bigger than the text they'll contain. But I'm not a designer :-) Maybe it would be worth getting feedback on the forums as well.

@hgiesel
Copy link
Contributor Author

hgiesel commented May 29, 2021

Hm, I agree that being spaced too far apart is ugly, but I'd argue that the dead space on the left and right are worse :)
The dead space makes it feel like, the options window should actually not be resizable, because it doesn't adapt to the window size.

There are some options to better accomodate the wider space. One would be to use have a scrollspy, which appears on the left as a navigation element at a certain width, and as a positive side-effect restricts the width of ConfigEditor.

We could also frame the options in a way, which makes it easier to associate them, below's an example of a Chrome preferences, which do this

Screenshot 2021-05-29 at 11 59 20

@hgiesel hgiesel force-pushed the deckoptionssections2 branch 2 times, most recently from 7d3ed34 to 7e36372 Compare May 29, 2021 16:17
@hgiesel hgiesel marked this pull request as ready for review May 30, 2021 23:14
@dae
Copy link
Member

dae commented Jun 2, 2021

I've given the code changes a quick review, and they look good. Regarding the design changes being discussed on the forums, we'll presumably want to merge this first in either case, due to all the other changes as well. If the consensus is for a design like #1212, will it be easy to adjust this to that design?

Some other issues I noticed:

  • the revert button is difficult to see in day mode
  • the left and right margins are too big on mobile now. Before:

IMG_1347

after:

IMG_1346

@hgiesel hgiesel force-pushed the deckoptionssections2 branch 2 times, most recently from edab766 to ac96574 Compare June 4, 2021 21:54
@hgiesel
Copy link
Contributor Author

hgiesel commented Jun 4, 2021

the left and right margins are too big on mobile now.

Which margins are you referring to exactly? The left margin of the main content seems to be the same to me. The right margin is bigger, because it leaves space for the revert button. I made the horizontal padding of the Revert Icon a bit smaller:

This is what it looks like on my phone now:

image

Which pointed me to another issue, when labels wrap. :)

If the consensus is for a design like #1212, will it be easy to adjust this to that design?

Actually no. With Boostrap Grid being built upon Flexbox, it cannot behave in such a way. But maybe it's for the better to move away from Bootstrap Grid. It was written before CSS Grid existed, and I read multiple blog posts about people moving away from it in favor of CSS Grid. But we might still be able to use some features of it, like the breakpoints. I'll give it a shot.

@dae
Copy link
Member

dae commented Jun 4, 2021

Hmm, @kleinerpirat seemed to think it would be a simple change?

The remaining margin issue is in the selector at the top - the former version extended the selector and buttons closer to the edges.

@dae
Copy link
Member

dae commented Jun 4, 2021

And the right margins are still different - the revert button previously closer to the edges (sort of like being in the "gutter")

@hgiesel
Copy link
Contributor Author

hgiesel commented Jun 5, 2021

And the right margins are still different - the revert button previously closer to the edges (sort of like being in the "gutter")

This is something I intentionally avoided, as the screen edges are also used for system functions on iOS, and to have more balanced design.

One Suggestion: Reverse input and revert position
image

@dae
Copy link
Member

dae commented Jun 5, 2021

I tried that in the past and found it looked a bit crowded, especially when the description text comes close to the input box. It can make it harder to hit the right button in that case as well.

Not sure what you mean by "system functions"? Swipes yes, but this is just a single tap, and the location is not far off the right arrow disclosure indicator shown in things like Settings.app.

@dae
Copy link
Member

dae commented Jun 5, 2021

(I do like how the warning is correctly balanced in that case though)

@hgiesel
Copy link
Contributor Author

hgiesel commented Jun 5, 2021

What do you think about putting the tooltip on the label instead, and possibly indicating with an underline. A dashed underline is also a common way to indicate a tooltip. And the label is certainly easier to hit than the info badge.

image

@dae
Copy link
Member

dae commented Jun 5, 2021

It's kind of ugly isn't it? :-) Especially when the descriptions are longer. I thought about doing something only on hover, but then it's not discoverable on mobile devices. IIRC I also ran into difficulty with popper putting the popup arrow in the wrong place when the entire text was the trigger.

I do like how it makes things neater (the info icons are a bit ugly too), but the conclusion I came to when originally implementing this was that they were the least bad option.

@dae
Copy link
Member

dae commented Jun 5, 2021

(Maybe if the indicator were more subtle it wouldn't be as bad?)

@hgiesel
Copy link
Contributor Author

hgiesel commented Jun 5, 2021

I also agree not a piece of art, but thought it was on the same level as having two icons per row :). Regarding making it more beautiful, we could decrease the opacity:

image

I don't see any other way right now. (Ignore the revert icons having switched the position again :) )

Regarding the positioning of the tooltips on mobile: they now typically touch the left edge of the device, which looks a bit off, but could be probably fixed by applying margins. I couldn't observe anything else.

@dae
Copy link
Member

dae commented Jun 5, 2021

That's not too bad, and am happy to give it a try in an alpha to see what people think. I still think it would be nice if the top area collapsed the margins when the screen size is very small so that more of the deck config title can fit on the screen - it's a navigation area so I don't think it needs to follow the same indent as the rest of the page.

@dae
Copy link
Member

dae commented Jun 5, 2021

Hmm, one other option would be to collapse the margins in the main area as well when the screen is too narrow. On small mobile devices we don't want much of a margin as horizontal space is at a premium, and the left margin was only there to match the right margin, which was only there to allow for the revert icon. So if the revert icon is being moved to the left, we should be able to get away with just a very small margin on left and right, that expands out once a reasonable width has been reached.

Presumably having the main area behave the same as the top area would make things easier too?

@kleinerpirat
Copy link
Contributor

kleinerpirat commented Jun 5, 2021

Hmm, @kleinerpirat seemed to think it would be a simple change?

I agree with Henrik that a grid would be more complicated, but I believe you are actually referring to the two-column-layout I showed on the forum, which would indeed be super easy to implement, as it doesn't use grid at all.

All I did for that was to wrap half of the Items in a <div class="left">, the other half in <div class="right"> and added three CSS rules beneath:

    @media (min-width: 992px) {
        .left {
            width: 48%;
            float: left;
            position: relative;
        }
        .right {
            width: 48%;
            float: right;
        }
        .left::after {
            content: "";
            position: absolute;
            border-left: 1px solid var(--medium-border);
            height: 95%;  /* arbitrary */
            /* That calc aligns the vertical line with the horizontal ones
            and was adjusted for a font size of 1.5em */
            top: calc(24px + 1.5em);  
            right: -4%;
        }
    }

Regarding the revert buttons: You could hide most of their ugliness by only showing the one whose corresponding input is focused/hovered.
Kooha-2021-06-05-15_29_35

Codepen for that approach:

https://codepen.io/kleinerpirat/pen/dyvevQv

Now, you might think "What about the incrementors?". To that I would say move them out of the input to the right as a separate element (2 stacked buttons). In the forum I read some criticism because they only show on hover. That would fix this issue.

Furthermore, they don't adjust to night mode currently:
Screenshot from 2021-06-05 15-37-55

@hgiesel
Copy link
Contributor Author

hgiesel commented Jun 5, 2021

All I did for that was to wrap half of the Items in a

, the other half in
and added three CSS rules beneath:

I'd want to avoid having to split the sections into left/right, as it is supposed to be extensible (and the API for it already exists, it's the same as for extending the buttons in the editor).

Moving the revert button into the input sounds fine with me, even though we need to make sure they are still easy to hit, when on mobile (I think I even tried to do that in the beginning, hmm...). The incrementor/step buttons are just a browser default, and they could be hidden, but are otherwise hard to modify browser-wide. They're also currently not displayed on mobile (as they would probably be hard to hit).

@kleinerpirat
Copy link
Contributor

I'd want to avoid having to split the sections into left/right, as it is supposed to be extensible

Ah I understand.

Perhaps there is a dynamic way to do that splitting.

With grid, one would need to set a max-height to tell it when to wrap to the next column.
That's certainly a bit problematic. Hm..

Moving the revert button into the input sounds fine with me, even though we need to make sure they are still easy to hit, when on mobile.

I think this would be a smart move. I didn't notice any issues on mobile with my codepen.

With that change, there would be enough room for the info buttons again. Although the underlines are intuitive, they make the text harder to read and look a bit off, because usually this style is only used on single-word definitions.

@hgiesel
Copy link
Contributor Author

hgiesel commented Jun 5, 2021

Hm, however where to put the revert button for the checkboxes then... The checkbox does not need to focused / active, to change its value...

Edit: We could activate the revert button on hover of the column containing the checkbox and the label. I'll do some experiments... (Edit: however that also wouldn't work on mobile...)

@kleinerpirat
Copy link
Contributor

kleinerpirat commented Jun 5, 2021

Oh yes. Didn't think about that...
Also, for <select> elements this isn't that straightforward either.

I'll do some experiments...

Curious what you come up with. But if it doesn't work, reveal on hover is not that essential, right?
As long as the icons are within the <input> elements, we save a lot of space already - and it looks tidier. Checkboxes don't take a lot of room anyway. It's just the <select>'s that bother me a bit... Where would you put the revert button on those? To the left of the arrow? Or to the right of the whole thing?


Sadly I got a big exam coming up in three weeks, so I don't have much time to experiment on this one.

@hgiesel
Copy link
Contributor Author

hgiesel commented Jun 5, 2021

So, the gutters cannot be entirely avoided, as they're built into Bootstrap Grid. If we're going to abandon Bootstrap Grid in favor of CSS Grid, this will be fixed anyway. However I'd like to do that in a new PR. Things I want to tackle:

  • Use CSS Grid in favor of Bootstrap Grid. I might need one or two attempts to get it right, and I do not want to muddy the PR.
  • Make "Revert tooltip" use Bootstrap Tooltip
  • Maybe we could expose the Revert button as a an IconButton on the left side of the number input rather than a free floating badge, or make it absolute positioned on the left edge of the number input. Not sure which approach is better here.

@hgiesel
Copy link
Contributor Author

hgiesel commented Jun 21, 2021

I hopefully addressed the issues. The revert button now has a gear icon and is positioned on the left.

IMO the important part here is that the gear icon is not invisible if the setting is reset, so it cannot cause an unbalanced line.

Screenshot 2021-06-21 at 22 43 04

However I also seem to have discovered a bug in svelte-check. It says createTooltip is not used / is not defined, which is not the case.

@dae
Copy link
Member

dae commented Jun 21, 2021

Thanks Henrik. I still have some niggles about the revert button, but I don't want to hold this up any more, so I'll merge it in as-is and open up a new PR for discussion.

@dae dae merged commit 76b0059 into ankitects:main Jun 21, 2021
@hgiesel hgiesel deleted the deckoptionssections2 branch October 20, 2021 13:12
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.

None yet

3 participants