[Horizontal Layout] Implement st.container() by akrolsmir · Pull Request #1961 · streamlit/streamlit

akrolsmir · 2020-09-02T11:04:42Z

Building off of the existing infrastructure for DeltaGenerator._block(). This serves as the basis for other kinds of linear layout (e.g. columns, expandable)

karriebear

Mostly me struggling with naming and hierarchy but looks good. A lot of super nitpicky comments to hopefully help with readability 🤞

On another note, do we want to merge into develop (aka can it be released in two weeks)? If we want to hold it off from releasing, we could also just comment out the container = _main.container in DG. If we do want to merge into develop, I think we should update docs.

One thing that components did that I liked and should have done for file uploader is a feature branch on streamlit and merge our forks into that branch instead of develop. Then when we're ready to ship the project, merge feature branch in.

karriebear · 2020-09-02T14:28:26Z

frontend/src/App.tsx

I guess it would depend how you implement the spacing of columns. Would you need empty containers to space/position them evenly?

Uh -- we might? Of the top of my head, columns should equally spaced containers inside a horizontal flex div, so the empty container still preserves space.

Mostly this is me noting: I don't know why we remove empty containers, and should think deeper about it.

karriebear · 2020-09-02T15:11:55Z

frontend/src/lib/utils.ts

super nitpicky but it'd be great to give meaningful variable names to the accumulative output from a reduce rather than acc.

Trying flattened; lmk if that doesn't make sense

pretty much anything works! it was partly me not realizing we were in a reduce and me trying to figure out what acc stood for before i connected all the dots.

karriebear · 2020-09-02T15:18:53Z

frontend/src/lib/DeltaParser.ts

Suggested change

// A list of nodes to render (e.g. main, sidebar, st.container, st.column)

// A list of nodes to render. Example BlockElement includes main, sidebar, st.container, st.column

karriebear · 2020-09-02T16:17:37Z

frontend/src/lib/DeltaParser.ts

Suggested change

container (derived from parentBlockContainer) is the container for each DeltaGenerator. This is not related to the container created as part of `st.container()`

Sorry nitpicky and slightly confused by the multiple use of the word container. When I first saw this, I was thinking this is weird, what about st.container? It wasn't until I dug into how DG worked that I figured it out... Feel free to reword and could also move this up to where parentBlockContainer declared? 🤷‍♀️

Not nitpicky at all -- the feedback that this is confusing is immensely valuable. And it was confusing to me as well! I just spent too long staring at it and it became normal, the same way "DeltaGenerator" almost seems like a normal word now.

So, let's not call "main|sidebar" a "Container". I'm trying out "topLevelBlock" here; alternatives considered include <root/topLevel/parent> + <block/container/element>

I like root but I'm also good with topLevelBlock. I trust you with the naming!

karriebear · 2020-09-02T16:38:25Z

lib/tests/streamlit/delta_generator_test.py

are we allowing nesting?

Mm few related thoughts on that:

From a product side, containers in containers may be okay? Columns in columns is the really weird/bad thing I think.

st.columns is probably going to be done with nested containers, so we'll need to figure out a distinction between "user nesting" and "Streamlit nesting"

Disallowing nesting can be done in a separate PR, to keep the scope of this one a bit smaller.

I think 3) is the best consideration for not thinking too hard about it now!

karriebear · 2020-09-02T16:58:31Z

frontend/src/lib/DeltaParser.ts

Suggested change

// Build the full path to a delta: [1, 0, 2] => [1, "element", 0, "element", 2]

// Gets the full path to the delta ReportElement within a container: [1, 0, 2] => [1, "element", 0, "element", 2]

Mostly because I didn't understand the hierarchy but I was really confused as to what this deltaPath was for. It may be more readable if we put elements[container] into a typed variable and reference that instead of container. Rewording welcomed!

Is it better when container => topLevelBlock? We can replace the get with a typed variable, but we'll still need to refer to Elements["main"] for the set -- and at that point I'm hoping that a repetition of elements["main"] = elements["main"].setIn(...) is a clearer signal of what we're doing, which is updating the Main element tree.

"Elements" is also a bad name for "pointer to Main and Sidebar nodes", honestly.

karriebear · 2020-09-02T17:05:01Z

frontend/src/lib/DeltaParser.ts

Should we put ☝️ into an interface and declare the map with that instead of any? Mostly because I see us creating a report element below without any tie-in

Yeah... I'd like this to be typed, but ImmutableMap doesn't play that well with typing afaict, which is why I just put it in a comment. Some thoughts on pros/cons of ImmutableJS here:

Reasons to use ImmutableMap:

Immutability guarantee is nice

getIn([0, "element", 2]) is nice for tree traverse, we'd have to implement it ourselves otherwise

Reasons not to use ImmutableMap:

No type safety

Weird pattern: reading object.get('property') is harder than object.property

No autocomplete =(

Debugging is harder (console.log(ImmutableMap) is worthless)

What the internet has to say about typing immutable maps (tldr no easy solution):
*https://blog.mayflower.de/6630-typescript-redux-immutablejs.html,
*immutable-js/immutable-js#683

I did not realize immutable did not like types. I believe I saw that ReportElements added to state where immutability would be great and since we have to traverse the tree, it makes sense to be untyped in favor of immutability

akrolsmir · 2020-09-02T21:54:10Z

On another note, do we want to merge into develop (aka can it be released in two weeks)? If we want to hold it off from releasing, we could also just comment out the container = _main.container in DG. If we do want to merge into develop, I think we should update docs.

One thing that components did that I liked and should have done for file uploader is a feature branch on streamlit and merge our forks into that branch instead of develop. Then when we're ready to ship the project, merge feature branch in.

I was originally thinking to just merge it in and not tell our users, rather than having a long-running branch. Mostly cuz I hate resolving merge conflicts. But I totally forgot about docs; since they're not yet ready (and probably want some product review), not merging into develop/ sounds reasonable. I'll set up a horiz-layout branch for this!

* try updating in virtualenv * new cache * am i using 39.0.1 and if so why is it failing? * somewhere somehow we are upgrading to setuptools 50.0.3... but where..? * looks like somewhere between setup + pipenv-install we get a newer version of setuptools * looks like somewhere between setup + pipenv-install we get a newer version of setuptools * force reinstall * looks like it's test requirements messing us up? * force reinstall + cleanup debugs

* [Horizontal Layout] Implement st.container() (#1961) * Fix up dg._block() to be dg.container() * Add a unit test * Add an e2e test for st.container * Minor cleanups * Clean up variable names and comments * Clean up deltaPath code and docs * [Horizontal Layout] Implement `with DG` (#1964) * Force reinstall of setuptools to 49.6.0 (#1962) * try updating in virtualenv * new cache * am i using 39.0.1 and if so why is it failing? * somewhere somehow we are upgrading to setuptools 50.0.3... but where..? * looks like somewhere between setup + pipenv-install we get a newer version of setuptools * looks like somewhere between setup + pipenv-install we get a newer version of setuptools * force reinstall * looks like it's test requirements messing us up? * force reinstall + cleanup debugs * Fix up dg._block() to be dg.container() * Add a unit test * Add an e2e test for st.container * Minor cleanups * Clean up variable names and comments * Clean up deltaPath code and docs * Organize DGs in a tree, to track the `with DG` * Enable `with` in every st.foo call * Appease mypy with # type: ignore * More sacrifices at the altar of mypy * Add unit and e2e tests for with * Actually, don't rely on each DG mixin to call _active_dg Revert "Appease mypy with # type: ignore" This reverts commit 4ba7c47. Revert "Enable `with` in every st.foo call" This reverts commit 9b467d6. * Use the active DG for all "protected" class methods * Clean up DG docs * Don't switch DGs from add_rows Per Karrie's suggestion Co-authored-by: karrie <[email protected]> * [horiz-layout] Add collapsible_container (#1978) * Add collapsible_container * Add default collapsed * make label required + initial docs * Consolidate with WIP column work * Address PR comments for more cleanliness * Update e2e tests * Rename collapsible container (#1993) * rename to expander * Fix tests * Rename collapsible to expandable * [Horiz Layout] Columns (#1979) * Implement columns with variable widths * Add a e2e test for columns * Add a unit test * Update Prettier to 1.19 to allow optional chaining https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Optional_chaining * Fix e2e test * Downgrade Prettier back to 1.18 * Empty columns now take up space * Remove duplicate function from merge conflict * Use flex-gap instead of margin, so column sides don't get padded * Minor tweaks requested by code review * Prefix new layout methods with `beta_` Co-authored-by: karrie <[email protected]>

akrolsmir requested a review from a team September 2, 2020 11:04

karriebear approved these changes Sep 2, 2020

View reviewed changes

akrolsmir changed the base branch from develop to horiz-layout September 2, 2020 22:01

karriebear and others added 7 commits September 2, 2020 18:19

Fix up dg._block() to be dg.container()

1301b23

Add a unit test

fa9dd4e

Add an e2e test for st.container

6fb8812

Minor cleanups

82635df

Clean up variable names and comments

87a78fd

Clean up deltaPath code and docs

66c8ba5

akrolsmir force-pushed the container branch from 44ffe8a to 66c8ba5 Compare September 3, 2020 00:26

akrolsmir merged commit 725efaa into streamlit:horiz-layout Sep 3, 2020

karriebear mentioned this pull request Sep 3, 2020

A st.group() or st.row() Container would be helpful. #1062

Closed

karriebear linked an issue Sep 3, 2020 that may be closed by this pull request

A st.group() or st.row() Container would be helpful. #1062

Closed

	// A list of nodes to render (e.g. main, sidebar, st.container, st.column)
	// A list of nodes to render. Example BlockElement includes main, sidebar, st.container, st.column


	container (derived from parentBlockContainer) is the container for each DeltaGenerator. This is not related to the container created as part of `st.container()`

	// Build the full path to a delta: [1, 0, 2] => [1, "element", 0, "element", 2]
	// Gets the full path to the delta ReportElement within a container: [1, 0, 2] => [1, "element", 0, "element", 2]

Conversation

akrolsmir commented Sep 2, 2020

Uh oh!

karriebear left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akrolsmir Sep 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karriebear Sep 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akrolsmir commented Sep 2, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

karriebear left a comment •

edited

Loading

akrolsmir Sep 2, 2020 •

edited

Loading

karriebear Sep 2, 2020 •

edited

Loading