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: [M3-7980] - Update PlacementGroups linodes tooltip and SelectPlacementGroup option label #10408

Merged
merged 4 commits into from
Apr 26, 2024

Conversation

abailly-akamai
Copy link
Contributor

@abailly-akamai abailly-akamai commented Apr 25, 2024

Description 📝

Small PR to bring a couple UI updates to the Placement Groups

Changes 🔄

List any change relevant to the reviewer.

  • Change PlacementGroupSelect option label to have the affinity type floating right and remove affinity type from selected option
  • Improve PG table row linodes tooltip (linking, placement)

Preview 📷

Before After
Screenshot 2024-04-25 at 13 24 59 Screenshot 2024-04-25 at 13 25 34
Screenshot 2024-04-25 at 13 28 03 Screenshot 2024-04-25 at 13 27 47

How to test 🧪

Verification steps

ℹ️ Perform these verifications in Alpha

  • Verify the PlacementGroupSelect change in the linode create flow (select Newark region)
  • Verify the tooltip changes at http://localhost:3000/placement-groups (make sure to have a PG and a Linode assigned to it)

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

@abailly-akamai abailly-akamai self-assigned this Apr 25, 2024
@abailly-akamai abailly-akamai marked this pull request as ready for review April 25, 2024 17:35
@abailly-akamai abailly-akamai requested a review from a team as a code owner April 25, 2024 17:35
@abailly-akamai abailly-akamai requested review from mjac0bs and bnussman-akamai and removed request for a team April 25, 2024 17:35
Copy link

github-actions bot commented Apr 25, 2024

Coverage Report:
Base Coverage: 81.82%
Current Coverage: 81.82%

Comment on lines 75 to 95
const optionLabel = (placementGroup: PlacementGroup, selected?: boolean) => (
<Stack
alignItems="center"
direction="row"
flex={1}
position="relative"
width="100%"
>
<Stack component="span">{placementGroup.label}</Stack>{' '}
<Stack
sx={{
position: 'absolute',
right: selected ? 14 : 34,
whiteSpace: 'nowrap',
}}
component="span"
>
({AFFINITY_TYPES[placementGroup.affinity_type]})
</Stack>
</Stack>
);
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to live here? Could it just be baked into PlacementGroupSelectOption? If so, I think it could clean things up a lot.

Is the absolute positioning needed? I think I was able to create a similar UI with a lot less one-off styling and positioning.

  const optionLabel = (placementGroup: PlacementGroup, selected: boolean) => (
    <Stack
      alignItems="center"
      direction="row"
      flexGrow={1}
      justifyContent="space-between"
      pr={selected ? 2 : undefined}
    >
      <Stack>{placementGroup.label}</Stack>{' '}
      <Stack>({AFFINITY_TYPES[placementGroup.affinity_type]})</Stack>
    </Stack>
  )

I didn't thoroughly test that code, just quickly looked for ways to clean up. Absolute positioning feels a bit scary to me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did you have to deal with the "selected" icon when doing so?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and yeah, i'll move it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW 'absolute' really isn't bad if your immediate parent is 'relative'. it's quite safe

Copy link
Member

Choose a reason for hiding this comment

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

I was pretty proud of how clean I was able to get the ImageOptionv2 component if you're looking for something to compare to., although, your select has the special "max capacity tooltip" which will definitely add some complexity.

Good to know about absolute + relative. That stuff is foreign to me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Improved things a bit around there, still had some dynamic positioning to do but it "looks" cleaner. Using quotes here cause yeah, the code may look cleaner to you, but sometimes I also care more about the output, and in those cases using a <Stack flexGrow={1} /> (aka adding another useless div) to achieve some basic spacing feels quite worse than using good ol' CSS done well.

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.

Confirmed the two changes. Left a suggestion about the PG select.

Unrelated to this PR, I did notice some spacing issues when the Region field shows an error. (I am assuming that the error is intended at this point, also.) Not sure if an existing ticket exists for this -- and maybe it should be a Gecko one.

Edit: It looks like in M3-7949, which is paused but part of Gecko GA, we intend to get rid of the Edge icon anyway and separate out the region select into two tabs, so I don't think we'll need to worry about this long-term, though I will mention it to Hana.

Screenshot 2024-04-25 at 12 44 09 PM

@@ -123,12 +123,16 @@ export const PlacementGroupsDetailPanel = (props: Props) => {
mb: 1,
width: '100%',
}}
textFieldProps={{
tooltipClasses: 'poo',
Copy link
Member

Choose a reason for hiding this comment

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

Is this class used? 💩

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤣 you have found my one debugging secret

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot 2024-04-26 at 11 43 18

@abailly-akamai abailly-akamai merged commit a0dad15 into linode:develop Apr 26, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants