Migrate Modal to Baseweb by kmcgrady · Pull Request #1930 · streamlit/streamlit

kmcgrady · 2020-08-27T19:47:57Z

Issue: #1862

Description:
Migrate Modal from Reactstrap to Baseweb

Before

After

Before

After

Modal needed a header to make things look consistent and work visually.
Before

After

Before

After

Before

After

Before

After

Before

After

Before

After

Before

After

Made the modal a little wider

Contribution License Agreement

By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

tvst · 2020-08-28T05:14:07Z

Is it possible to make the style more closely match the original?

See comments on the image:

This text is getting too close to the X:

If I record a screencast then stop recording via the menu, I get the recorded file dialog but it's huuuuuge:
(it's hard to tell from this screenshot because the dialog is the entire screen. But you can tell by looking at the top of the screen and noticing that the dialog header is there)

This progressbar looks wrong too:

And for some reason the "Copy to clipboard" button is not vertically aligned with the other ones. It's a couple of pixels off:

Edit: actually it's apparently using the old Bootstrap button:

Also:

If I record a screencast then press Esc to stop recording, I don't see the dialog that shows the recorded file. My guess is the dialog is handling the Esc event prematurely and closing.
Buttons are missing the "active" state. They should turn pink when clicked.
Checkboxes should be pink when checked
Checkbox focus ring should be pink, not black
Not sure if from this PR, but the menu item focus ring should be pink (right now it's black, but I don't remember it existing at all in the past...)

tvst · 2020-08-28T05:43:21Z

Out of all of these, the most important ones are:

Make sure all the colors match our theme
Make sure the Modal title is aligned with the X button
Make sure the Copy to Clipboard button uses Baseweb
Fix screencast Esc issue

As for border radii, lines, sizes, etc, those are less important. If they're hard to do, we can probably skip (need to see it live!)

karriebear

Here's some initial comments on the components. I assume the styles will get updated based on product comments and can take a look at the styling after

frontend/src/components/core/StreamlitDialog/StreamlitDialog.tsx

karriebear · 2020-08-28T06:17:25Z

frontend/src/components/shared/Button/Button.tsx

+export enum Kind {
+  PRIMARY = "primary",
+  SECONDARY = "secondary",
+}


Curious why we have this? BaseUI's KIND enum has these + 2 more and in Modal.tsx we're using both enums

This is more a precursor to the Button migration and preparation for a design system. We talked about this for the Alert conversion to have our own enum instead of using BaseWeb. I was migrating the same stuff. Since we only use two versions, I didn't add more, but we can certainly in the future

frontend/src/components/shared/Modal/Modal.tsx

karriebear · 2020-08-28T06:27:46Z

frontend/src/components/shared/Modal/Modal.tsx

+const primary = SCSS_VARS.$primary
+const primaryA50 = SCSS_VARS["$primary-a50"]
+const textMargin = SCSS_VARS["$font-size-sm"]
+const white = SCSS_VARS.$white


It could just be my preference but I didn't like doing this so I made the colors from widgetTheme exportable. If you pull in latest you can use that instead of reassigning values here.

Awesome. Will use

karriebear · 2020-08-28T06:29:59Z

frontend/src/components/shared/Modal/Modal.tsx

+function ModalHeader({ children }: ModalHeaderProps): ReactElement {
+  return (
+    <UIModalHeader>
+      <span className="ModalTitle">{children}</span>


According to our styleguide, we like camelCasing though that could all change

Awesome. I'll make the change to conform for now.

karriebear · 2020-08-28T06:47:33Z

frontend/src/components/shared/Modal/Modal.tsx

+    },
+  }
+
+  const buttonKind = isPrimary ? KIND.primary : KIND.tertiary


Not sure if it's me but using tertiary here seems weird 🤷‍♀️ probably because I don't think we have a tertiary style in our brand. I think we can probably leave it and then when we do a whole design pass through, we can clean it up.

More of a FYI, the default minimal button kind matches tertiary. Not sure if that makes it better or worse but I'm using minimal for some fun icon buttons

Wasn't sure where this goes (this comment or the one above). I think this is weird with the future proofing we're in the process of but I could live with this in the meantime. Linking to #1860?

That sounds reasonable. I chose tertiary, cause visually it matches what we are looking for.

karriebear · 2020-08-28T06:53:50Z

frontend/src/components/shared/Modal/Modal.tsx

+
+function ModalButton({
+  children,
+  kind,


Should we change this to isPrimary? It seems we don't actually care what kind of button, only if it's primary or not. Knowing that we overwrite this if it's not primary makes it odd when you declare a different kind of button like here

Or perhaps the thing to do is to update the buttonSecondary colors so that when we say we want secondary, we get secondary.

Sorry a lot of comment overlap between here and 👇 ☝️

I guess I was future proofing it to support multiple kinds. Baseweb supports 4 kinds, and we may want to support all 4 as well. In addition, I think if we use ModalButton as essentially a Button with that margin update, I'd like their interface to be the same.

frontend/src/components/core/StreamlitDialog/StreamlitDialog.tsx

karriebear · 2020-09-03T05:13:40Z

This looks to also fixes #1740

karriebear

Looks good. Can't wait to see everything transitioned over!

The only comment that gives me some concern is the one regarding class nesting for VideoRecordedDialog but not sure how that ties into the overall styling work that's going to happen. Risk of an issue arising in the meantime is pretty low.

karriebear · 2020-09-03T00:10:19Z

frontend/src/App.tsx


    // The c key clears the cache.
-    c: (): void => this.openClearCacheDialog(),
+    CLEAR_CACHE: (): void => this.openClearCacheDialog(),


not annoying enough since keyMap is right above but the comments could be updated as well. more of a nice to have

karriebear · 2020-09-03T06:04:48Z

frontend/src/components/shared/Modal/Modal.tsx

+    },
+  }
+
+  const buttonKind = isPrimary ? KIND.primary : KIND.tertiary


Wasn't sure where this goes (this comment or the one above). I think this is weird with the future proofing we're in the process of but I could live with this in the meantime. Linking to #1860?

karriebear · 2020-09-03T06:20:37Z

frontend/src/hocs/withScreencast/components/VideoRecordedDialog/style.scss


 @import "src/assets/css/theme";

-.streamlit-dialog.screencast {


I'm not sure this is the right move? While this is imported specifically in the VideoRecordedDialog component, this still gets compiled into a css file for the page. It does not look like we do [the fancy webpack term] with dynamic loading. With this change, these styles would then get applied to all other modal bodies. While most of them either make sense globally or is random enough, .download-button could be problematic in the future. If we do prefer this to ax the .streamlit-dialog.screencast parent, I would vote to move this into our assets folder instead for future developers to not go wondering why does my download-button have a margin-top though whole other topic you'll be addressing :)

oh. you're probably right. Let me relook into this.

karriebear · 2020-09-03T06:38:21Z

frontend/src/components/shared/Modal/Modal.scss

+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */


I think we can ax this file? 50% of this looks like it's covered in our body default, BaseWeb's theme and/or JS. Given that we're doing styling in JS also, can we standardize one way or the other? Fine to leave as is if this is a transitional state.

Yes. That's easy to address.

karriebear · 2020-09-03T16:55:51Z

frontend/src/hocs/withScreencast/components/VideoRecordedDialog/style.scss

-      }
+  .first-column,
+  .second-column,
+  .third-columnd {


sorry missed this on the first pass but also it's been like this so not sure if fixing the typo will actually cause more grief or not... the component itself does not have the typo so i guess we've been living without some y padding 🤷

Suggested change

.third-columnd {

.third-column {

tvst · 2020-09-03T20:54:11Z

Looks amazing!

Some final comments, no need to do another product pass once fixed:

The modal body sometimes overflows
The horizontal padding of the dialog body should match the one used in the title. (i.e. the title padding is the correct one)
Just curious: how come we're not using Styletron for styling these components? And why not use BaseWeb's themeing system, for that matter?

kmcgrady · 2020-09-03T21:30:54Z

Thanks @tvst ! Modals do not have any theme variables. I think Buttons may have some, and we do use some, but we have a rich override set that does most of the work.

In terms of Styletron, we are actually overriding the styles of these components, which is the only available way to override these. I think we haven't committed to a styled component engine. The truth is while Baseweb is great, I do not think we see a long term future in it. We do a lot of overrides that make the work more brittle. It's an open discussion though, so we didn't put much effort using it right now. If that changes, that will fall under the conforming to the style guide part of the project.

* develop: Migrate Modal to Baseweb (streamlit#1930) Update bl dependency to resolve CVE (streamlit#1965) Force reinstall of setuptools to 49.6.0 (streamlit#1962) ComponentInstance unit tests (streamlit#1956) Update change log Fix alignment of `.. output::` command in st.write() (streamlit#1955) Fix alignment of `.. output::` command in st.write() (streamlit#1955) Add spacing to slider docs Up version to 0.66.0 Switch statement to include ArrowTable (streamlit#1951) Remove st.write from unsupported in sidebar (streamlit#1952) Remove st.write from unsupported in sidebar (streamlit#1952) Switch statement to include ArrowTable (streamlit#1951) Add deprecation label to st.deck_gl_chart (streamlit#1943) Pin setuptools to <= 49.6.0 (streamlit#1944) DuplicateWidgetID error message should surface widget name (streamlit#1942) Set z-index of balloons to 1000000 to allow it to display over other elements. (streamlit#1934)

Migrate Modal to Baseweb

c038cc8

kmcgrady requested a review from a team August 27, 2020 19:47

Merge branch 'develop' into modal-1862

307e12b

kmcgrady added the product review needed label Aug 27, 2020

Fixed test

a49c857

karriebear self-requested a review August 28, 2020 05:55

karriebear reviewed Aug 28, 2020

View reviewed changes

Ken McGrady added 5 commits September 2, 2020 10:17

Updated look and feel from modals

76eceb2

Merge branch 'develop' into modal-1862

097f274

Addressed code review comments

0bed9d6

Fixed test

b7cfd7c

Merge branch 'develop' into modal-1862

61b230a

karriebear approved these changes Sep 3, 2020

View reviewed changes

Addressed Code Review Comments

c706f4c

karriebear reviewed Sep 3, 2020

View reviewed changes

karriebear approved these changes Sep 3, 2020

View reviewed changes

Addressed typo

8ec85c7

tvst removed the product review needed label Sep 3, 2020

Addressed Product Review issues

d580525

kmcgrady merged commit 41948f4 into streamlit:develop Sep 3, 2020

kmcgrady deleted the modal-1862 branch September 3, 2020 23:33


		@import "src/assets/css/theme";

		.streamlit-dialog.screencast {

Conversation

kmcgrady commented Aug 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tvst commented Aug 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tvst commented Aug 28, 2020

Uh oh!

karriebear left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

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

Uh oh!

Uh oh!

karriebear left a comment

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!

tvst commented Sep 3, 2020

Uh oh!

kmcgrady commented Sep 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kmcgrady commented Aug 27, 2020 •

edited

Loading

tvst commented Aug 28, 2020 •

edited

Loading

karriebear commented Sep 3, 2020 •

edited

Loading