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

feat: [UIE-8074] - DBaaS GA Summary tab #11091

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

corya-akamai
Copy link
Contributor

@corya-akamai corya-akamai commented Oct 11, 2024

Description 📝

DBaaS GA summary tab refactor. Data remains the same, although the layout is altered.

Changes 🔄

  • V1 Summary components moved to /legacy dir marked deprecated and will be removed post GA release.
  • V2 Summary components with tests

Target release date 🗓️

10/28/24

Preview 📷

V2 Legacy V2 Default
v2-legacy v2-default

How to test 🧪

Prerequisites

  • Managed Databases account capability
  • LD flag: beta: false, enabled: true

Verification steps

  • Toggle LD flag will show GA screen vs. V1 screen

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

@corya-akamai corya-akamai requested a review from a team as a code owner October 11, 2024 17:36
@corya-akamai corya-akamai requested review from bnussman-akamai and jaalah-akamai and removed request for a team October 11, 2024 17:36
@mjac0bs mjac0bs added the DBaaS Relates to Database as a Service label Oct 11, 2024
Copy link

github-actions bot commented Oct 11, 2024

Coverage Report:
Base Coverage: 86.99%
Current Coverage: 87%

@cpathipa cpathipa requested review from cpathipa and removed request for jaalah-akamai October 11, 2024 18:11
Comment on lines +42 to +61
export const StyledValueBox = styled(Box, {
label: 'StyledValueBox',
})(({ theme }) => ({
'& > button': {
marginTop: '-5px',
},
'& > div': {
alignSelf: 'flex-start',
verticalAlign: 'sub',
},
'& button.MuiButtonBase-root': {
marginTop: '0',
},
'& button.copy-tooltip': {
marginTop: '-3px',
},
color: theme.palette.mode === 'dark' ? theme.color.grey8 : theme.color.black,
display: 'flex',
padding: '3px 5px',
}));
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we do something like the following to improve alignment (vertically centered values), make use of flex, and avoid the need for another nested component within the Grid item by replacing the parent of StyledValueBox with:

Suggested change
export const StyledValueBox = styled(Box, {
label: 'StyledValueBox',
})(({ theme }) => ({
'& > button': {
marginTop: '-5px',
},
'& > div': {
alignSelf: 'flex-start',
verticalAlign: 'sub',
},
'& button.MuiButtonBase-root': {
marginTop: '0',
},
'& button.copy-tooltip': {
marginTop: '-3px',
},
color: theme.palette.mode === 'dark' ? theme.color.grey8 : theme.color.black,
display: 'flex',
padding: '3px 5px',
}));
export const StyledGridValue= styled(Grid2, {
label: 'StyledGridValue',
})(({ theme }) => ({
alignItems: 'center',
color: theme.palette.mode === 'dark' ? theme.color.grey8 : theme.color.black,
display: 'flex',
padding: `0 ${theme.spacing()}`,
}));

I was seeing some weirdness with the icons existing outside the grid container at really small screen sizes (which is also currently happening), so might need to make some adjustments to this still.

Screenshot 2024-10-16 at 4 07 16 PM

Comment on lines +199 to +211
const readOnlyHostValue =
database?.hosts?.standby ?? database?.hosts?.secondary ?? '';

const readOnlyHost = () => {
const defaultValue = isLegacy ? '-' : 'not available';
const value = readOnlyHostValue ?? defaultValue;
return (
<>
{value}
<CopyTooltip
className={classes.inlineCopyToolTip}
text={readOnlyHostValue}
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Screenshot 2024-10-16 at 3 16 57 PM

I think we'll want the copy tooltip not to render if the readOnlyHostValue is an empty string.

Copy link
Contributor

Choose a reason for hiding this comment

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

CI has a typecheck error that I'm not seeing locally. 🤔
Screenshot 2024-10-16 at 4 21 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.

Unrelated to the above, I am also seeing eslint warnings in several files. Can we clean those up before merging this?

image

The VSCode ESLint extension (if you don't already have it installed) is very useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DBaaS Relates to Database as a Service
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants