Convert RefreshWidget to Mantine, CSS modules, and TS #42768

oisincoveney · 2024-05-16T13:37:56Z

Refactors the RefreshWidget to use Mantine popovers, CSS modules, and typescript. I made this change since the logic in Popover was breaking when displaying the static dashboard, and the height wasn't being calculated properly. I figured that using the Mantine popover would be an easier way to go since the component wasn't that big, and I figured it would be a better use of our time than trying to figure out why the Popover is working in our dashboards but not in the SDK.

Since I was also in the area, I converted the component to TS. I also converted the emotion to CSS modules since there were some issues with styling in the Embedding SDK that I thought would be solved by using CSS modules instead of emotion. That wasn't the issue but I think it's a welcome conversion too.

oisincoveney · 2024-05-16T13:52:04Z

frontend/src/metabase/components/icons/CountdownIcon.tsx

I converted this one to TS as well to fix the type error in RefreshWidget, where it was asking for a className that's not necessarily needed

oisincoveney · 2024-05-16T13:53:29Z

frontend/src/metabase/css/core/colors.module.css

@@ -4,6 +4,7 @@
 :root {
  --color-white: #fff;
  --color-success: #84bb4c;
+  --color-summarize: #88bf4d;


Needed to add this to match color("summarize")

Could you please change it to be

Suggested change

--color-summarize: #88bf4d;

--mb-color-summarize: #88bf4d;

replay-io · 2024-05-16T14:25:19Z

Status	Complete ↗︎
Commit	`41feff5`
Results	⚠️ 4 Flaky ✅ 2523 Passed

heypoom

Looks good! I haven't tried to run this locally yet, but code-wise looks good. Left some minor comments and questions.

heypoom · 2024-05-17T12:17:52Z

frontend/src/metabase/dashboard/components/RefreshWidget/RefreshWidget.tsx

+}: {
+  setRefreshElapsedHook?: (hook: (elapsed: number | null) => void) => void;
+  period: number | null;
+  onChangePeriod: (period: number | null) => void;


nit: wondering if we have a naming convention on this? (e.g. the props in the above diff is onRefreshPeriodChange, so this could be onPeriodChange too or vice-versa)

One thing that I'm doing with #42777 will be to standardize this. Don't worry, it's definitely a thought I've had as well 😄

heypoom · 2024-05-17T12:21:58Z

frontend/src/metabase/dashboard/components/RefreshWidget/RefreshWidget.tsx

+  useUpdateEffect(() => {
+    if (
+      setRefreshElapsedHook &&
+      prevProps?.setRefreshElapsedHook !== setRefreshElapsedHook
+    ) {
+      setRefreshElapsedHook((elapsed: number | null) => {
+        setElapsed(elapsed);
+      });
+    }
+  });


I wonder why we use usePrevious in this case instead of using standard useEffect to track dependency change on setRefreshElapsedHook?

I basically copied the code over from JS. Usually when I try to use something like a new useEffect in place of the old code something breaks, but I've changed it and things seem to be working correctly still. I've made the change

Oh sorry, I thought this was a new logic - forgot for a moment that this is only for TS migration, my bad 🙏🏻

I'm not exactly sure if useEffect would cover all the edge cases though (that was just me being curious), but if it works fine then ok!

heypoom · 2024-05-17T12:23:09Z

frontend/src/metabase/dashboard/components/RefreshWidget/RefreshWidget.module.css

+.RefreshWidgetTitle {
+  color: var(--color-text-medium);
+  font-weight: bold;
+  font-size: 0.75em;
+  text-transform: uppercase;
+  margin-bottom: 1em;
+  margin-left: 0.5em;
+}


I'm curious on when do we decide to write our own styles in CSS Modules, as opposed to using metabase/ui Mantine components? (Mantine components can specify most of these props)

I think the main blocker is nested selectors, as you see later in the file. I've moved the independent classes to the mantine props as you suggested, but some of them will have to stay for now

Got it! I think it makes complete sense to go with CSS modules for nested selectors, as there is no other way to handle it 😄

Mantine's style props is much more limited than, say, what tailwind and friends can do unfortunately. I'm not a big fan of their use-hover shenanigans either

heypoom · 2024-05-17T12:24:23Z

frontend/src/metabase/dashboard/components/RefreshWidget/RefreshWidgetTarget.tsx

+
+import { CountdownIcon } from "metabase/components/icons/CountdownIcon";
+import { DashboardHeaderButton } from "metabase/dashboard/components/DashboardHeader/DashboardHeader.styled";
+import { isNotNull } from "metabase/lib/types";


(not related to this PR, but I thought lib/types is about types, I'm surprised to see functions here)

deniskaber · 2024-05-17T13:02:59Z

frontend/src/metabase/css/core/colors.module.css

@@ -4,6 +4,7 @@
 :root {
  --color-white: #fff;
  --color-success: #84bb4c;
+  --color-summarize: #88bf4d;


Could you please change it to be

Suggested change

--color-summarize: #88bf4d;

--mb-color-summarize: #88bf4d;

deniskaber · 2024-05-17T13:03:51Z

frontend/src/metabase/dashboard/components/RefreshWidget/RefreshOption/RefreshOption.tsx

@@ -0,0 +1,27 @@
+import cx from "classnames";
+
+import RefreshOptionS from "metabase/dashboard/components/RefreshWidget/RefreshOption/RefreshOption.module.css";


Let's use relative import here

Suggested change

import RefreshOptionS from "metabase/dashboard/components/RefreshWidget/RefreshOption/RefreshOption.module.css";

import RefreshOptionS from "./RefreshOption.module.css";

github-actions · 2024-05-17T13:25:00Z

@oisincoveney Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone?

Convert RefreshWidget to mantine and TS

b210370

oisincoveney changed the title ~~Convert RefreshWidget to Mantine and TS~~ Convert RefreshWidget to Mantine, CSS modules, and TS May 16, 2024

metabase-bot bot assigned oisincoveney May 16, 2024

metabase-bot bot added the .Team/Embedding label May 16, 2024

oisincoveney requested a review from a team May 16, 2024 13:39

Fix TS error

77c5565

oisincoveney commented May 16, 2024

View reviewed changes

Add missing color

87888c0

oisincoveney commented May 16, 2024

View reviewed changes

Split components

f59f8c0

oisincoveney added the no-backport Do not backport this PR to any branch label May 16, 2024

heypoom approved these changes May 17, 2024

View reviewed changes

heypoom requested a review from a team May 17, 2024 12:26

oisincoveney added 2 commits May 17, 2024 15:41

Move some styles to mantine and use useEffect instead of prevProps

431d2d9

Merge branch 'master' into convert-refresh-widget-to-mantine-ts

41feff5

oisincoveney enabled auto-merge (squash) May 17, 2024 13:00

deniskaber approved these changes May 17, 2024

View reviewed changes

oisincoveney merged commit 2cf2955 into master May 17, 2024
109 checks passed

oisincoveney deleted the convert-refresh-widget-to-mantine-ts branch May 17, 2024 13:24

oisincoveney added a commit that referenced this pull request May 17, 2024

Suggestions from #42768

17a7f60

oisincoveney mentioned this pull request May 17, 2024

Review suggestions from #42768 #42830

Merged

oisincoveney added a commit that referenced this pull request May 17, 2024

Review suggestions from #42768 (#42830)

01e45b8

oisincoveney added a commit that referenced this pull request May 20, 2024

Review suggestions from #42768 (#42830)

7a0cc24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert RefreshWidget to Mantine, CSS modules, and TS #42768

Convert RefreshWidget to Mantine, CSS modules, and TS #42768

oisincoveney commented May 16, 2024 •

edited

oisincoveney May 16, 2024

oisincoveney May 16, 2024

deniskaber May 17, 2024

replay-io bot commented May 16, 2024 •

edited

heypoom left a comment

heypoom May 17, 2024

oisincoveney May 17, 2024

heypoom May 17, 2024

oisincoveney May 17, 2024

heypoom May 17, 2024 •

edited

heypoom May 17, 2024

oisincoveney May 17, 2024

heypoom May 17, 2024 •

edited

heypoom May 17, 2024 •

edited

oisincoveney May 17, 2024

deniskaber May 17, 2024

deniskaber May 17, 2024

github-actions bot commented May 17, 2024

		@@ -0,0 +1,27 @@
		import cx from "classnames";

		import RefreshOptionS from "metabase/dashboard/components/RefreshWidget/RefreshOption/RefreshOption.module.css";

	import RefreshOptionS from "metabase/dashboard/components/RefreshWidget/RefreshOption/RefreshOption.module.css";
	import RefreshOptionS from "./RefreshOption.module.css";

Convert RefreshWidget to Mantine, CSS modules, and TS #42768

Convert RefreshWidget to Mantine, CSS modules, and TS #42768

Conversation

oisincoveney commented May 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

replay-io bot commented May 16, 2024 • edited

heypoom left a 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

heypoom May 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heypoom May 17, 2024 • edited

Choose a reason for hiding this comment

heypoom May 17, 2024 • 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

github-actions bot commented May 17, 2024

oisincoveney commented May 16, 2024 •

edited

replay-io bot commented May 16, 2024 •

edited

heypoom May 17, 2024 •

edited

heypoom May 17, 2024 •

edited

heypoom May 17, 2024 •

edited