new tabs part 1 (widgetry demo, jump-to-time, and trips table) #571

michaelkirk · 2021-03-19T05:48:39Z

We have several "tab" like systems in the app.

This PR works on consolidate those implementations, and incorporates some of the new designs.

As a reminder, here's the tab spec from figma:

The active tab has dark text, while the inactive tabs which you can switch to have faded text.

I chose to diverge from figma a bit, primarily because the "inactive" tabs were tough to read with the low-contrast text styling, especially on the translucent background in a way that wasn't obvious from figma.

before

The "finished", "canceled", "unfinished" tabs:

The time warp:

after

increasing contrast

One other thing I did, which might be controversial...

Previously the panel_bg and the panel_section were the same colors, but the panel_bg had some opacity applied. This often looked good, but if the map behind the panel has a bunch of content which is almost panel colored, then the contrast between panel_bg and panel_section is lost.

In particular, this could make the tabs hard to differentiate.

To address this, I doped the panel_bg to be a bit farther away from the section color. Compare the tab outline in the left image (doped) vs. right (pure).

TODO: - tooltips (or is that just a responsibility of the button builder?) - widgetry color scheme should use day-theme - pick nice tab colors - start applying! - jump in time - info panel - "more data" - fix width to match widest? Or maybe width modes?

The GenericTable machinery lost a lot of it's utility now that we have proper tabs. I've left a couple of free functions to be shared, and inlined the remaining simple functionality.

Currently the color was only via transparency - so depending on what was behind the panel, there was sometimes not enough contrast.

michaelkirk · 2021-03-19T05:51:20Z

game/src/sandbox/dashboards/misc.rs

-    }
-
-    fn draw(&self, g: &mut GfxCtx, app: &App) {
-        g.clear(app.cs.dialog_bg);


Oh, I also got rid of the green backgrounds, because it felt more cohesive. Is there a reason we should keep it?

Relatedly, I don't really understand what draw_baselayer is about, but once I removed the g.clear here, I observed some glitchy rendering until I also removed DrawBaselayer::Custom.

I also got rid of the green backgrounds, because it felt more cohesive

Agreed, still seeing the map underneath is nicer. The "fullscreen" design with the green background was just meant to draw focus away from the map and towards the data, I think, but the new design is just as easy to read, and more cohesive as you say.

I don't really understand what draw_baselayer is about

widgetry manages a stack of states. Most of the time, the current state is totally responsible for drawing everything, including initially clearing the screen of whatever was previously drawn there. Sometimes, we want to ask the previous state to draw, then draw on top of that. Most of the time, we want to do something default (SharedAppState::draw_default) -- in this case, draw the map. Occasionally though, we want to avoid the (negligible) perf hit of rendering the map at all, so we use Custom, which means we're responsible for doing g.clear too.

So what you've done is go back to DefaultDraw, which just renders the map, then draws on top of it. Totally fine!

michaelkirk · 2021-03-19T05:52:47Z

game/src/sandbox/dashboards/parking_overhead.rs

+            DashTab::ParkingOverhead.picker(ctx, app),
+            Widget::col(vec![
+                Widget::row(vec![
+                    Text::from_multiline(vec![


No changes to the content, other than that's it's now outlined and has "section" styling.

michaelkirk · 2021-03-19T05:53:33Z

game/src/sandbox/dashboards/summaries.rs

                    Widget::col(vec![
                        summary_boxes(ctx, app, &filter),
                        Widget::row(vec![
-                            contingency_table(ctx, app, &filter),
-                            scatter_plot(ctx, app, &filter),
+                            contingency_table(ctx, app, &filter).bg(ctx.style().section_bg),


Unrelated to the tabs, but while I was here, I noticed a few things that should probably have "section" styling.

michaelkirk · 2021-03-19T05:56:55Z

widgetry/src/style/button_style.rs

-            .image_color(solid_style.fg, ControlState::Default)
-            .image_bg_color(solid_style.bg, ControlState::Default)
-            .image_bg_color(solid_style.bg_hover, ControlState::Hovered)
+            .image_color(self.btn_solid.fg, ControlState::Default)


previously we used self.btn_tab to style things like the icon portion of the "pop up" buttons (e.g. "switch scenario"), but the new tab styling isn't compatible with that usage, so I've introduced another button_style: btn_solid.

michaelkirk · 2021-03-19T05:58:10Z

widgetry/src/widgets/panel.rs

@@ -437,6 +437,12 @@ impl Panel {
        }
    }

+    pub fn swap_contained_content(&mut self, ctx: &EventCtx, name: &str, widget: &mut Widget) {


Not sure about this name... maybe swap_inner_content would be better?

I'm a bit confused about what this is doing. Why is the first member of the row or column special and worth swapping out?
EDIT: Ah, it's because we're assuming name was made from a .container() call and ONLY has one item. Is it worth asserting that there's only one item in there below?

API-wise: maybe lock it down to pub(crate) unless you have use cases outside of TabController in mind, and add a bit of docstring?

michaelkirk · 2021-03-19T06:02:25Z

The info panel behomoth, and a few other places we do tab-ish things (challenge picker, scenario picker) are still TODO, but I wanted to have an opportunity to check in on direction before going further - though these changes are intended to be mergeable as is.

dabreegster · 2021-03-19T16:52:53Z

Reviewing the changes functionally (digging into the code next): this looks great! I love how the trip table now retains state as you click between different tabs. Before, if I fiddled with filters and sorting, switched tabs, then returned, everything would be reset. It's much more intuitive to remember where we left off -- nice!

I noticed a regression with buttons on the scenario picker:

increasing contrast

This change is easier to read/distinguish, so SGTM.

michaelkirk · 2021-03-19T17:56:04Z

I noticed a regression with buttons on the scenario picker:

I agree it looks pretty bad, but to my eye, not worse than the current look:

Can you clarify what change you'd like to see here?

Is it that you want the buttons to continue to have a visible border/background rather than appearing as "plain"?

dabreegster · 2021-03-19T16:59:24Z

game/src/info/mod.rs

                .build_def(ctx),
        );
        hyperlinks.insert(name.to_string(), link);
    }
    {
-        // stop-gap color that is semi-legible across themes until the tab redesign is completed


The new color is hard to distinguish as tabs at all, or to see which is selected. I'm assuming this will get changed out with the new tab design soon, so fine for now.

Even though I haven't put the info-tabs into the new tab system, design wise, it's already pretty close. The only visual difference would be adding the "section" background to the tab's content, instead of a translucent background.

That might help a little bit, but probably not much. Do you find the contrast in the "more data" page tabs to be sufficiently legible?

This contrast issue is why I offset the panel_bg in ecca637

I could increase that effect to increase contrast, but it's such a key part of the overall design that I'm trying to diverge only as much as necessary to get readable content.

Do you find the contrast in the "more data" page tabs to be sufficiently legible?

Yes, but possibly only because the tabs happen to be on top of a section with the background, so the current tab kind of merges in:

It still feels hard to distinguish for me, but I'm fine with sticking close to the design for now. Thanks!

dabreegster · 2021-03-19T17:01:49Z

game/src/sandbox/dashboards/generic_trip_table.rs

-    }
-}
-
-impl<T: 'static, F: 'static, P: 'static + Fn(&mut EventCtx, &App, &Table<App, T, F>) -> Panel>


I am thrilled to see this gutted in favor of something more generic. Wooot!

dabreegster · 2021-03-19T17:06:45Z

game/src/sandbox/dashboards/misc.rs

-    }
-
-    fn draw(&self, g: &mut GfxCtx, app: &App) {
-        g.clear(app.cs.dialog_bg);


I also got rid of the green backgrounds, because it felt more cohesive

Agreed, still seeing the map underneath is nicer. The "fullscreen" design with the green background was just meant to draw focus away from the map and towards the data, I think, but the new design is just as easy to read, and more cohesive as you say.

I don't really understand what draw_baselayer is about

widgetry manages a stack of states. Most of the time, the current state is totally responsible for drawing everything, including initially clearing the screen of whatever was previously drawn there. Sometimes, we want to ask the previous state to draw, then draw on top of that. Most of the time, we want to do something default (SharedAppState::draw_default) -- in this case, draw the map. Occasionally though, we want to avoid the (negligible) perf hit of rendering the map at all, so we use Custom, which means we're responsible for doing g.clear too.

So what you've done is go back to DefaultDraw, which just renders the map, then draws on top of it. Totally fine!

dabreegster · 2021-03-19T17:07:43Z

game/src/sandbox/dashboards/mod.rs

@@ -18,9 +18,7 @@ mod trip_table;
 // Oh the dashboards melted, but we still had the radio


You were so close to my Modest Mouse reference, but no comment about it? :P

All post-2000 MM references are lost on me, but I'll check it out as penance.

dabreegster · 2021-03-19T17:09:56Z

game/src/sandbox/dashboards/parking_overhead.rs

-use widgetry::{EventCtx, Filler, Line, Panel, State, Text, Toggle, Widget};
+use widgetry::{EventCtx, Filler, GfxCtx, Line, Outcome, Panel, State, Text, Toggle, Widget};
+
+type Transition = widgetry::Transition<App>;

 use crate::app::App;


You can pull in crate::app::Transition here instead of the typedef above

dabreegster · 2021-03-19T17:26:05Z

game/src/sandbox/dashboards/trip_table.rs

+                } else if self.table_tabs.handle_action(ctx, &x, &mut self.panel) {
+                    // if true, tabs handled the action
+                } else {
+                    todo!("unhandled action: {}", x)


nit: panic instead of todo, since this should logically be unreachable in its current form, right?

dabreegster · 2021-03-19T17:38:20Z

game/src/sandbox/dashboards/trip_table.rs

+    fn event(&mut self, ctx: &mut EventCtx, app: &mut App) -> Transition {
+        match self.panel.event(ctx) {
+            Outcome::Clicked(x) => {
+                if self.table_tabs.active_tab_idx() == 0 && self.finished_trips_table.clicked(&x) {


I really like the improvements to the code here! Using push_tab to build them up and keeping all of the tabs "alive" at all times is quite intuitive. There's a little bit of boilerplate to associate the tab index with the content and route events, but I think it's very localized and easy to understand. The alternatives are likely much more bizarre to express in Rust, with callbacks or implementing some complicated trait. So I like this!

Looking forward to applying this to info panels in particular, there might be some more complications with conceptually keeping all tabs' content "active" at the same time. Some tabs auto-update as time passes, and this can be a bit expensive, so we wouldn't want to necessarily do it for multiple tabs. Also they fill out this weird Details struct to draw extra zoomed/unzoomed stuff on the map, and it wouldn't make sense to draw multiple tabs' extra content at once. Maybe we just wind up using the new tab button style there, but for the moment, leave the existing weird structures in place for updating/switching tabs.

dabreegster · 2021-03-19T17:40:49Z

game/src/sandbox/time_warp.rs

+                    if self.tabs.handle_action(ctx, action, &mut self.panel) {
+                        // if true, tabs has handled the action
+                    } else {
+                        todo!("handle action: {}", action)


I'm guessing this was an intermediate step of developing the tab controller. Should we change this to an assert or something else now that this case should be unreachable?

dabreegster · 2021-03-19T17:50:43Z

widgetry/src/widgets/panel.rs

@@ -437,6 +437,12 @@ impl Panel {
        }
    }

+    pub fn swap_contained_content(&mut self, ctx: &EventCtx, name: &str, widget: &mut Widget) {


I'm a bit confused about what this is doing. Why is the first member of the row or column special and worth swapping out?
EDIT: Ah, it's because we're assuming name was made from a .container() call and ONLY has one item. Is it worth asserting that there's only one item in there below?

API-wise: maybe lock it down to pub(crate) unless you have use cases outside of TabController in mind, and add a bit of docstring?

dabreegster · 2021-03-19T17:51:16Z

widgetry/src/widgets/table.rs

@@ -136,6 +144,9 @@ impl<A, T, F> Table<A, T, F> {
            make_table(ctx, headers, rows, 0.88 * ctx.canvas.window_width),
            make_pagination(ctx, num_filtered, self.skip),
        ])
+        .named(&self.id)
+        // return in separate container incase caller want to apply an outter-name


incase -> in case
outter -> outer

dabreegster · 2021-03-19T17:59:14Z

Is it that you want the buttons to continue to have a visible border/background rather than appearing as "plain"?

The current look definitely is jarring. But yes, just looking like plain text makes it less clear it's a button. Wouldn't a slight outline or background work?

michaelkirk · 2021-03-19T18:16:28Z

I'm a bit confused about what this is doing. Why is the first member of the row or column special and worth swapping out?

I've renamed this method and its params and added a docstring.

dabreegster · 2021-03-19T18:47:09Z

LGTM! Thanks for the PR. Let me know if you want to talk over how to apply this to info panels.

michaelkirk added 8 commits March 18, 2021 11:03

use daytheme in widgetry demo

165d50c

style tabs

3a92351

wip apply tabs to TimeWarp

3c41f74

fixup! style tabs

b2aacfd

fixup! wip apply tabs to TimeWarp

4553fd1

convert trip tables to new tabs

650b74e

The GenericTable machinery lost a lot of it's utility now that we have proper tabs. I've left a couple of free functions to be shared, and inlined the remaining simple functionality.

increase contrast of panel_bg vs the panel_section

ecca637

Currently the color was only via transparency - so depending on what was behind the panel, there was sometimes not enough contrast.

michaelkirk commented Mar 19, 2021

View reviewed changes

fixup! convert trip tables to new tabs

a02320c

michaelkirk commented Mar 19, 2021

View reviewed changes

michaelkirk requested a review from dabreegster March 19, 2021 05:59

dabreegster reviewed Mar 19, 2021

View reviewed changes

michaelkirk added 3 commits March 19, 2021 11:06

Code review: prefer unreachable to todo for checkins

6246b84

CR: clarify swap_inner_content method

f083cd5

CR: use already-defined transition

8e9f2aa

michaelkirk added 2 commits March 19, 2021 11:25

extract "percent" function, put header text first

fb33242

increase tab contrast a bit more in dark theme

1391202

dabreegster merged commit 6218043 into a-b-street:master Mar 19, 2021

michaelkirk mentioned this pull request Mar 19, 2021

scenario picker #575

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new tabs part 1 (widgetry demo, jump-to-time, and trips table) #571

new tabs part 1 (widgetry demo, jump-to-time, and trips table) #571

michaelkirk commented Mar 19, 2021

michaelkirk Mar 19, 2021

dabreegster Mar 19, 2021

michaelkirk Mar 19, 2021

michaelkirk Mar 19, 2021

michaelkirk Mar 19, 2021 •

edited

michaelkirk Mar 19, 2021

dabreegster Mar 19, 2021

michaelkirk commented Mar 19, 2021

dabreegster commented Mar 19, 2021

michaelkirk commented Mar 19, 2021

dabreegster Mar 19, 2021

michaelkirk Mar 19, 2021 •

edited

dabreegster Mar 19, 2021

dabreegster Mar 19, 2021

dabreegster Mar 19, 2021

dabreegster Mar 19, 2021

michaelkirk Mar 19, 2021

dabreegster Mar 19, 2021

michaelkirk Mar 19, 2021

dabreegster Mar 19, 2021

michaelkirk Mar 19, 2021

dabreegster Mar 19, 2021

dabreegster Mar 19, 2021

michaelkirk Mar 19, 2021

dabreegster Mar 19, 2021

dabreegster Mar 19, 2021

dabreegster commented Mar 19, 2021

michaelkirk commented Mar 19, 2021

dabreegster commented Mar 19, 2021

		@@ -18,9 +18,7 @@ mod trip_table;
		// Oh the dashboards melted, but we still had the radio

new tabs part 1 (widgetry demo, jump-to-time, and trips table) #571

new tabs part 1 (widgetry demo, jump-to-time, and trips table) #571

Conversation

michaelkirk commented Mar 19, 2021

before

after

increasing contrast

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelkirk Mar 19, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelkirk commented Mar 19, 2021

dabreegster commented Mar 19, 2021

michaelkirk commented Mar 19, 2021

Choose a reason for hiding this comment

michaelkirk Mar 19, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dabreegster commented Mar 19, 2021

michaelkirk commented Mar 19, 2021

dabreegster commented Mar 19, 2021

michaelkirk Mar 19, 2021 •

edited

michaelkirk Mar 19, 2021 •

edited