upcoming: [DI-20800] - CSS for widget size control and feedbacks by venkymano-akamai · Pull Request #10951 · linode/manager

venkymano-akamai · 2024-09-17T08:13:16Z

Description 📝

CSS updates according to UX feedback for widgets and main bar

Changes 🔄

Fixed widget paper wrapper height
Fixed legend table height and overflow on scroll
Removed divider between filters and graph inside widget
Main Bar consistency according to UX margin (24px), divider color update
Placeholder update according to suggestion

Target release date 🗓️

01-10-2024

Preview 📷

Before	After

How to test 🧪

As some of endpoints are not available enable mock service
Go to monitors tab
Select dashboard and filters, you will be able to see widgets and CSS fixes

As an Author I have considered 🤔

Check all that apply

…nd margin updates

cpathipa

@venkymano-akamai Left couple comments in the initial review, Will take a deeper look later today. Please revolve the conflicts.

cpathipa · 2024-09-17T16:22:09Z

packages/manager/src/features/CloudPulse/Dashboard/CloudPulseDashboardLanding.tsx


  return (
-    <Grid container spacing={2}>
+    <Grid container paddingTop={'8px'} spacing={2}>


Can we use theme values instead of hardcoded values here?

done, changed to paddingTop={1} = 8px, also tried to use theme.spacing wherever possible

cpathipa · 2024-09-17T16:22:39Z

packages/manager/src/features/CloudPulse/Overview/GlobalFilters.tsx

+          <Divider
            sx={{
-              marginBlockEnd: 'auto',
+              borderColor: '#F4F5F6',


@cpathipa , the color '#F4F5F6' is not present in themes, i have used grey5 that seems to be very similar, any chance here we can go with '#F4F5F6' this color itself

@gitkcosby/ @cpathipa We have got this color from Dominik( ACLP UX), Would you able to help us to provide the appropriate theme color for this?

@venkymano-akamai Domink confirmed that we are good to use grey5 (#f7f7fa) as its almost same color. We can use the same.

Thanks!!, grey5 we will go

…ure/widget_size_controls # Conflicts: # packages/manager/src/features/CloudPulse/Utils/CloudPulseWidgetUtils.ts # packages/manager/src/features/CloudPulse/Widget/CloudPulseWidget.tsx # packages/manager/src/features/CloudPulse/Widget/components/Zoomer.tsx

This reverts commit c8615b2.

jaalah-akamai

Looks good overall - just a few comments

jaalah-akamai · 2024-09-19T13:07:06Z

packages/manager/src/features/CloudPulse/Utils/CloudPulseWidgetUtils.ts

+    height: theme.spacing(67.5), // 540px
+    maxHeight: theme.spacing(67.5), // 540px
+  },
+}));


I don't think any of this is necessary now that we've set an overflow/scroll for the legends.

@jaalah-akamai , we fixed this height because, we need to show error, while there is an issue from metrics API or any un-related errors, where we don't render the graph at all

Even in the error case, the spacing should remain the same

Yea so when I see maxHeight, that's usually a red flag for me because we're cropping the natural growth of elements (unless it's absolutely necessary). You just want the error state to always match the other containers, and that can be accomplished without these styles by adding this:

Replaced StyledWidgetWrapper with just Paper

In CloudPulseWidget.tsx

@jaalah-akamai , this works as well, I have removed the wrapper. Thanks for the suggestion

packages/manager/src/features/CloudPulse/Dashboard/CloudPulseDashboardWithFilters.tsx

packages/manager/src/features/CloudPulse/Widget/components/CloudPulseLineGraph.tsx

packages/manager/src/assets/icons/arrow_right.svg

packages/manager/src/features/CloudPulse/Widget/CloudPulseWidget.tsx

jaalah-akamai · 2024-09-19T13:30:39Z

packages/manager/src/features/CloudPulse/Widget/CloudPulseWidget.tsx

              direction={{ sm: 'row' }}
-              gap={{ md: 2, xs: 1 }}
+              gap={2}
+              maxHeight={`calc(${theme.spacing(10)} + 5px)`}


maxHeight - Doesn't appear to be doing much, and do we need it? I don't think those controls should grow at all

@jaalah-akamai , yes we need , because if we introduce more filters, possibly include a dimension filter, and another thing to start listing down the selected filters, we can fix the height and introduce a scroll for views. Because, in one widget, i might select three dimension filter and start listing down that and in another widget i only might select two filters, again the size of the widget is growing dynamically and we will have two different sized widgets

There is a use case in future we need to support another control like split by within a widget, so there is a quite chance it can grow.

jaalah-akamai · 2024-09-19T13:58:47Z

I'm not sure if this is a bug or not, but with the new overflow/scrolling, can we keep the mobile layout the same as desktop? I actually think it looks better and it would enable us to remove a lot of breakpoint specific styles.

Before	After

venkymano-akamai · 2024-09-19T15:35:30Z

I'm not sure if this is a bug or not, but with the new overflow/scrolling, can we keep the mobile layout the same as desktop? I actually think it looks better and it would enable us to remove a lot of breakpoint specific styles.

Before After

@jaalah-akamai , if it is regarding the styles of legend rows it is actual behaviour of linegraph, this involves changes in actual line graph component i think, anyway as we are migrating to recharts, we don't want to touch the actual line graph component. Let me know your thoughts on this

Also to achieve this we need to the changes in LineGraph.styles.ts and remove all styles involving theme.breakpoints.down('md'). We hope this might have already handled if we go for recharts, so we don't need now

Also , if this is mandatory and no one is using linegraph, we can remove those styles and commit it, please confirm

venkymano-akamai · 2024-09-19T15:44:17Z

@jaalah-akamai , I addressed most of comments and replied my thoughts on some of the comments, can you please check once?

kmuddapo · 2024-09-19T16:52:23Z

I'm not sure if this is a bug or not, but with the new overflow/scrolling, can we keep the mobile layout the same as desktop? I actually think it looks better and it would enable us to remove a lot of breakpoint specific styles.
Before After

@jaalah-akamai , if it is regarding the styles of legend rows it is actual behaviour of linegraph, this involves changes in actual line graph component i think, anyway as we are migrating to recharts, we don't want to touch the actual line graph component. Let me know your thoughts on this

Also to achieve this we need to the changes in LineGraph.styles.ts and remove all styles involving theme.breakpoints.down('md'). We hope this might have already handled if we go for recharts, so we don't need now

Also , if this is mandatory and no one is using linegraph, we can remove those styles and commit it, please confirm

My recommendation is not to touch the existing linegraph as its a generic component and we anyway planned to migrate to recharts in next sprint (2 weeks from now) and any changes to linegraph might be a throw away code again.

dwiley-akamai

Verified described CSS fixes ✅
Code review ✅

packages/manager/src/features/CloudPulse/shared/CloudPulseDashboardFilterBuilder.tsx

packages/manager/src/features/CloudPulse/Widget/CloudPulseWidget.tsx

packages/manager/.changeset/pr-10951-upcoming-features-1726561825358.md

coliu-akamai

For some reason I'm seeing that the first two graphs take up the entire screen width, even on a large screen, vs the last two graphs - not sure if this expected? Is anyone else experiencing the same?

pic 1	pic 2

…et.tsx Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com>

…825358.md Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com>

…boardFilterBuilder.tsx Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com>

jaalah-akamai · 2024-09-19T21:32:18Z

Will merge if pipeline passes 👍

…anager into feature/widget_size_controls

…ure/widget_size_controls

kmuddapo · 2024-09-20T03:44:31Z

For some reason I'm seeing that the first two graphs take up the entire screen width, even on a large screen, vs the last two graphs - not sure if this expected? Is anyone else experiencing the same?

pic 1 pic 2

Yes!! Thats expected, User has the ability to Zoom in/Zoom out the widget, based on that size of the widget varies.

kmuddapo · 2024-09-20T03:44:49Z

Will merge if pipeline passes 👍

Thanks!! @jaalah-akamai

coliu-akamai · 2024-09-20T12:48:02Z

Yes!! Thats expected, User has the ability to Zoom in/Zoom out the widget, based on that size of the widget varies.

Awesome, thanks for clarifying! 🤦

github-actions · 2024-09-20T13:12:52Z

Coverage Report: ✅
Base Coverage: 86.94%
Current Coverage: 86.94%

venkymano-akamai added 2 commits September 16, 2024 18:49

upcoming: [DI-20585] - CSS for widget size control and feedbacks

f0b9207

upcoming: [DI-20585] - Some simple updates for using styled wrapper a…

b21c912

…nd margin updates

venkymano-akamai requested a review from a team as a code owner September 17, 2024 08:13

venkymano-akamai requested review from cpathipa and dwiley-akamai and removed request for a team September 17, 2024 08:13

venkymano-akamai marked this pull request as draft September 17, 2024 08:13

venkymano-akamai added 7 commits September 17, 2024 13:52

upcoming: [DI-20800] - SVG updates according to CDS 2.0

ed075c5

upcoming: [DI-20800] - Simple CSS minHeight updates

c4abd51

upcoming: [DI-20800] - Added changeset

061860c

upcoming: [DI-20800] - CSS updates for button width

291ba17

upcoming: [DI-20800] - Widget level height corrections

2423b65

upcoming: [DI-20800] - Mock update to have one more dimension

489e9c1

upcoming: [DI-20800] - Spacing improvement in grids

0ad7f9d

coliu-akamai assigned venkymano-akamai Sep 17, 2024

coliu-akamai added the Cloud Pulse label Sep 17, 2024

cpathipa reviewed Sep 17, 2024

View reviewed changes

venkymano-akamai added 12 commits September 18, 2024 09:46

upcoming: [DI-20800] - Use Theme spacing and colors

9c070d5

upcoming: [DI-20800] - Use calc for custom themes

561e9de

upcoming: [DI-20800] - Comments for height in px

40b390d

upcoming: [DI-20800] - revert unwanted file changes to latest develop

d5db90c

upcoming: [DI-20800] - Use theme spacing wherever applicable

c8615b2

Revert "upcoming: [DI-20800] - Use theme spacing wherever applicable"

4c975e5

This reverts commit c8615b2.

upcoming: [DI-20800] - Placeholder value updates

93941a0

upcoming: [DI-20800] - Style updates for widget

ca1db73

upcoming: [DI-20800] - SVG fill color update

39b433c

upcoming: [DI-20800] - useTheme values

2ae688c

upcoming: [DI-20800] - Reusable component padding updates

5390037

venkymano-akamai marked this pull request as ready for review September 19, 2024 11:56

jaalah-akamai reviewed Sep 19, 2024

View reviewed changes

upcoming: [DI-20800] - PR comments addressed

e7118f1

upcoming: [DI-20800] - Removed styled widget wrapper

7265a3a

jaalah-akamai approved these changes Sep 19, 2024

View reviewed changes

jaalah-akamai added the Add'tl Approval Needed Waiting on another approval! label Sep 19, 2024

dwiley-akamai approved these changes Sep 19, 2024

View reviewed changes

coliu-akamai reviewed Sep 19, 2024

View reviewed changes

jaalah-akamai and others added 4 commits September 19, 2024 17:30

Update packages/manager/src/features/CloudPulse/Widget/CloudPulseWidg…

445a6dd

…et.tsx Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com>

Update packages/manager/.changeset/pr-10951-upcoming-features-1726561…

1561c53

…825358.md Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com>

Update packages/manager/src/features/CloudPulse/shared/CloudPulseDash…

2a525aa

…boardFilterBuilder.tsx Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com>

Update packages/manager/src/features/CloudPulse/shared/CloudPulseDash…

5365e6d

…boardFilterBuilder.tsx Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com>

jaalah-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Sep 19, 2024

venkymano-akamai added 4 commits September 20, 2024 05:52

upcoming: [DI-20800] - UT updates

1b07842

Merge branch 'feature/widget_size_controls' of github.com:venkymano/m…

5189b59

…anager into feature/widget_size_controls

upcoming: [DI-20800] - UT updates

1d69416

Merge branch 'develop' of https://github.com/linode/manager into feat…

0b48797

…ure/widget_size_controls

upcoming: [DI-20800] - Reverting not needed changes

2ab97bd

jaalah-akamai merged commit 538d9e1 into linode:develop Sep 20, 2024

This was referenced Sep 25, 2024

Release v1.129.0 - release → staging #11006

Merged

Release v1.129.0 - staging → master #11013

Merged

Conversation

venkymano-akamai commented Sep 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description 📝

Changes 🔄

Target release date 🗓️

Preview 📷

How to test 🧪

As an Author I have considered 🤔

Uh oh!

cpathipa 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!

venkymano-akamai Sep 18, 2024 • 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!

jaalah-akamai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

venkymano-akamai Sep 19, 2024 • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

venkymano-akamai Sep 19, 2024 • 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!

kmuddapo Sep 19, 2024 • 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!

jaalah-akamai commented Sep 19, 2024

Uh oh!

venkymano-akamai commented Sep 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

venkymano-akamai commented Sep 19, 2024

Uh oh!

kmuddapo commented Sep 19, 2024

Uh oh!

dwiley-akamai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coliu-akamai left a comment

Choose a reason for hiding this comment

Uh oh!

jaalah-akamai commented Sep 19, 2024

Uh oh!

venkymano-akamai commented Sep 17, 2024 •

edited

Loading

cpathipa left a comment •

edited

Loading

venkymano-akamai Sep 18, 2024 •

edited

Loading

venkymano-akamai Sep 19, 2024 •

edited

Loading

venkymano-akamai Sep 19, 2024 •

edited

Loading

kmuddapo Sep 19, 2024 •

edited

Loading

venkymano-akamai commented Sep 19, 2024 •

edited

Loading

coliu-akamai commented Sep 20, 2024 •

edited

Loading