Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

upcoming: [DI-20709] - CSS updates for widget level filters for ACLP #10903

Merged
merged 23 commits into from
Sep 17, 2024

Conversation

venkymano-akamai
Copy link
Contributor

@venkymano-akamai venkymano-akamai commented Sep 9, 2024

Description 📝

Some simple CSS updates for reducing the height and font size of the widget level filter components.

Changes 🔄

  1. Updated the CSS of input props of the autocomplete components in widget level filters
  2. Updated the height of the zoom icon component
  3. Update the variant of typography to h2 in widget heading title.
  4. Updated the multiselect components to not show placeholder if a item is selected
  5. Updated the widget component by removing the divider

Target release date 🗓️

16-09-2024

Preview 📷

Before After
Screenshot 2024-09-09 at 10 09 10 AM Screenshot 2024-09-09 at 10 10 10 AM
Screenshot 2024-09-09 at 10 17 12 AM Screenshot 2024-09-09 at 10 35 48 AM
Screenshot 2024-09-09 at 10 36 01 AM

How to test 🧪

  1. Login as mock user as some of endpoints are not production ready
  2. Navigate to the monitor page
  3. Select a linode dashboard and all required filters
  4. You will be able to see the widgets with updated CSS.

As an Author I have considered 🤔

Check all that apply

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

@venkymano-akamai venkymano-akamai requested a review from a team as a code owner September 9, 2024 04:43
@venkymano-akamai venkymano-akamai requested review from mjac0bs and jaalah-akamai and removed request for a team September 9, 2024 04:43
input: {
fontSize: WIDGET_FILTERS_COMMON_FONT_SIZE,
},
minHeight: WIDGET_FILTERS_COMMON_HEIGHT,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, Since the minHeight of textFieldInput is by default 34px and it is same everywhere in our application. We need to pass both min height and height for our use case

@jaalah-akamai
Copy link
Contributor

A couple thoughts here, because I'm not convinced this is the right approach:

  • I'm not for defining global css styles in constant files. We're actually moving away from this in Q4 when we begin incorporating design tokens and removing hard-coded styles like this. Things like this should really come from the theme file. If there's an actual UX ask for a different size autocomplete then we need to find a more prop-driven way to handle that.
  • With that said, why do we want to reduce the height of the Autocomplete in this specific scenario? Is this a UX ask because it would differ from everywhere else we're using Autocompletes.
  • With these current changes, the alignment of the text within the Autocomplete is off
  • The typography title change to H2 is ok

@venkymano-akamai
Copy link
Contributor Author

@jaalah-akamai , Yes this is an UX ask for reducing the height of the autocomplete, and as per latest mockups it should be of 22px.

Happy to discuss a more suitable approach to achieve this without affecting the styles.

@venkymano-akamai
Copy link
Contributor Author

venkymano-akamai commented Sep 10, 2024

@jaalah-akamai , we have updated the implementation approach by, using styledautocomplete, that will set the minheight and padding for us, can you please look into this and let us know. If you have any new suggestions on the approach, please let us know

With these current changes, the alignment of the text within the Autocomplete is off - for this comment, i think there is not alignment impact since we are controlling the placement of text within autocomplete by padding. Adjusted the padding as well

Copy link

github-actions bot commented Sep 10, 2024

Coverage Report:
Base Coverage: 86.64%
Current Coverage: 86.63%

@gitkcosby
Copy link

Just speaking from the UX side, the autocomplete input height should not be adjusted. The UX designers on this project can resolve any alignment questions. In the event a new size or style variation for a given component is needed, we can address that.

@venkymano-akamai
Copy link
Contributor Author

Just speaking from the UX side, the autocomplete input height should not be adjusted. The UX designers on this project can resolve any alignment questions. In the event a new size or style variation for a given component is needed, we can address that.

@gitkcosby , we need to reduce the height of the Autocomplete to 22px, since the input's min height is 34px, we need to adjust the minHeight to achieve this, because, reducing the height of autocomplete to 22px, doesn't work here.

@kmuddapo
Copy link

Just speaking from the UX side, the autocomplete input height should not be adjusted. The UX designers on this project can resolve any alignment questions. In the event a new size or style variation for a given component is needed, we can address that.

@gitkcosby Is this applicable even for width? We shouldn't reduce it?

@venkymano-akamai
Copy link
Contributor Author

venkymano-akamai commented Sep 11, 2024

@jaalah-akamai / @gitkcosby , we have reverted the height changes and kept the original, now only we have reduced the width, let us know if this is okay.

Also added couple of changes for removing placeholder if an element is selected in a multi select filter component

Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seeing a typecheck CI error in a test file for an unused import, which will need to be fixed: https://github.com/linode/manager/actions/runs/10830866544/job/30068135825?pr=10903

Screenshot 2024-09-12 at 1 46 57 PM

And some additional feedback just from viewing the latest UI, since I hadn't in a little while. Are there tickets to address the following in upcoming PRs?

  • DbaaS is not capitalized consistently in the dashboard dropdown, reading Dbaas.

Screenshot 2024-09-12 at 1 40 41 PM

  • There are white space and padding issues with the legends in the graphs.

Screenshot 2024-09-12 at 1 35 49 PM

To reiterate - we shouldn't address these in this PR, but we will want to fix them and just want to make sure they're accounted for.

@@ -39,7 +45,7 @@ export const getInSeconds = (interval: string) => {
};

// Intervals must be in ascending order here
export const all_interval_options = [
export const all_interval_options: IntervalOptions[] = [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export const all_interval_options: IntervalOptions[] = [
export const allIntervalOptions: IntervalOptions[] = [

We use camel case for variable names throughout the codebase. Can we be consistent?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why we're not placing the options side by side and stacking them instead? There's a lot of empty space, so it looks a bit odd.

Screenshot 2024-09-12 at 1 35 44 PM

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They used to be longer, but with the changes this looks weird. You're correct, the grid will need to be adjusted now

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

@mjac0bs / @jaalah-akamai , for smaller screens we have made the autocomplete to occupy 100 percent width now, please check

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @venkymano-akamai, this looks much better on small screens. I left one comment about how to write that styling rule better to rely on our theme.

@venkymano-akamai
Copy link
Contributor Author

venkymano-akamai commented Sep 13, 2024

@mjac0bs / @jaalah-akamai ,

  1. Have addressed the CI error.
  2. Have made the drop down to occupy the entire width of row for easier selections.
  3. Also addressed the camelCase issue
  4. The legend rows will be updated in upcoming PR's, anyway we are moving to recharts as well.

Hope this is good now.

@jdamore-linode
Copy link
Contributor

jdamore-linode commented Sep 13, 2024

@venkymano-akamai any chance you can merge in the latest changes from develop? It contains a workaround for the API issue that's causing a lot of the test failures on this PR

Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@venkymano-akamai Thanks for the reminder about Recharts.

My feedback about Dbaas not being capitalized consistently in the Select still stands, and we should be using DBaaS in copy consistently. Can you provide me with a ticket number where that fix will be made?

Left a couple of more review recommendations on my last pass, but approving pending those are addressed and tests pass.

firstIntervalIndex,
all_interval_options.length
allIntervalOptions.length
);

let default_interval =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: default_interval existed before this PR, but same comment about preferring camel case over snake case to keep in mind when going forward.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Comment on lines 23 to 40
style={{
color: theme.color.grey1,
fontSize: 'x-large',
height: '34px',
}}
data-testid="zoom-in"
onClick={() => handleClick(false)}
style={{ color: 'grey', fontSize: 'x-large' }}
/>
);
}

return (
<ZoomOutMap
style={{
color: theme.color.grey1,
fontSize: 'x-large',
height: '34px',
}}
Copy link
Contributor

@mjac0bs mjac0bs Sep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use the sx prop here? (docs reference)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

label: 'StyledAutocomplete',
})(() => ({
'&& .MuiFormControl-root': {
'@media (max-width: 600px)': {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using our theme.breakpoint values is a better way to do this. For example:

export const StyledWidgetAutocomplete = styled(Autocomplete, {
  label: 'StyledAutocomplete',
})(({ theme }) => ({
  '&& .MuiFormControl-root': {
    minWidth: '90px',
    [theme.breakpoints.down('sm')]: {
      width: '100%', // 100% width for xs and small screens
    },
    width: '90px',
  },
}));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @venkymano-akamai, this looks much better on small screens. I left one comment about how to write that styling rule better to rely on our theme.

@venkymano-akamai
Copy link
Contributor Author

@mjac0bs , Thanks for the pass and addressed those pending comments as well.

@jaalah-akamai , Can we get a second pass?

@coliu-akamai coliu-akamai added the Approved Multiple approvals and ready to merge! label Sep 17, 2024
@venkymano-akamai
Copy link
Contributor Author

@jaalah-akamai , we have enough approvals, Can we merge it?

@jaalah-akamai jaalah-akamai merged commit 44763ea into linode:develop Sep 17, 2024
19 of 20 checks passed
nikhagra-akamai pushed a commit to nikhagra-akamai/manager that referenced this pull request Sep 23, 2024
…inode#10903)

* upcoming: [DI-20709] - CSS updates for widget level filters

* upcoming: [DI-20709] - ES lint fixes

* upcoming: [DI-20709] - Added comments

* upcoming: [DI-20709] - Added changeset

* upcoming: [DI-20709] - Color updates for zoomer component with use them hook

* upcoming: [DI-20585] - CSS changes

* upcoming: [DI-20585] - Removed unused values and imports

* upcoming: [DI-20585] - Removed unused values and imports

* upcoming: [DI-20585] - Use common utils

* upcoming: [DI-20585] - Comment updates

* upcoming: [DI-20585] - Remove any

* upcoming: [DI-20585] - Removing height and adjusting width. Removed divider in widget

* upcoming: [DI-20585] - Removed placeholder on selection and UT updates

* upcoming: [DI-20585] - UT updates

* upcoming: [DI-20585] - Using form control for width of filters

* upcoming: [DI-20585] - Alignment fix

* upcoming: [DI-20585] - PR comments

* upcoming: [DI-20585] - PR comments

* upcoming: [DI-20585] - Updated code syntax for handling small size screens

* upcoming: [DI-20585] - CamelCase for properties

* upcoming: [DI-20585] - Style to sx

---------

Co-authored-by: vmangalr <vmangalr@akamai.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge! Cloud Pulse
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants