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: [M3-8728] - Add Product Families to Create Menu dropdown #11260

Merged

Conversation

hana-akamai
Copy link
Contributor

@hana-akamai hana-akamai commented Nov 14, 2024

Description 📝

Add Product Families to the Create Menu Dropdown and display the desktop dropdown as 3 columns in a row; mobile will remain as a single column dropdown

Note: The Storage section was intentionally moved after Networking (different from the Side Nav Menu) since it has less items and can be in the same column as Databases. UX would prefer not to change the Side Nav order as it "reflects the info hierarchy from Ryan McEntee, that is also being used by TechDocs."

Changes 🔄

  • Add Product Families to Create Menu dropdown

Clean up 🧹 :

  • Deleted unused nav components
  • Renamed AddNewMenu to CreateMenu
  • Variable renaming in PrimaryNav
  • Updated unit test to use userEvent over fireEvent

Preview 📷

Before After
Screenshot 2024-11-15 at 3 32 55 PM
Screen.Recording.2024-11-15.at.2.57.41.PM.mov

How to test 🧪

Verification steps

  • Open the create menu and ensure you can tab through the items. Clicking links still work as expected, etc
  • Check mobile view
yarn test CreateMenu

As an Author, I have considered 🤔

  • 👀 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

  • I have read and considered all applicable items listed above.

As an Author, before moving this PR from Draft to Open, I confirmed ✅

  • All unit tests are passing
  • TypeScript compilation succeeded without errors
  • Code passes all linting rules

@hana-akamai hana-akamai added the UX/UI Changes for UI/UX to review label Nov 14, 2024
@hana-akamai hana-akamai self-assigned this Nov 14, 2024

import { CreateMenu } from './CreateMenu';

describe('CreateMenu', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file was renamed from AddNewMenu and replaced fireEvent with userEvent

Comment on lines +27 to +36
it('renders product family headings', async () => {
const { getAllByRole, getByText } = renderWithTheme(<CreateMenu />);
const createButton = getByText('Create');
await userEvent.click(createButton);
const headings = getAllByRole('heading', { level: 3 });
const expectedHeadings = ['Compute', 'Networking', 'Storage', 'Databases'];
headings.forEach((heading, i) => {
expect(heading).toHaveTextContent(expectedHeadings[i]);
});
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New addition

Comment on lines +68 to +80
const account = accountFactory.build({
capabilities: [],
});

server.use(
http.get('*/account', () => {
return HttpResponse.json(account);
})
);

const { getByText, queryByText } = renderWithTheme(<CreateMenu />, {
flags: { databases: false, dbaasV2: { beta: false, enabled: false } },
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test was failing so I added capabilities and flags to ensure isDatabasesEnabled is false in CreateMenu

description?: string;
}

export const CreateMenu = () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This component was renamed from AddNewMenu

Comment on lines 172 to 191
<Stack direction="column" tabIndex={-1}>
<ProductFamilyGroup
handleClose={handleClose}
productFamily={productFamilyLinkGroup[0]}
/>
</Stack>
<Divider
orientation="vertical"
sx={{ height: 'auto', margin: 0, marginTop: 1 }}
/>
<Stack direction="column" tabIndex={-1}>
<ProductFamilyGroup
handleClose={handleClose}
productFamily={productFamilyLinkGroup[1]}
/>
</Stack>
<Divider
orientation="vertical"
sx={{ height: 'auto', margin: 0, marginTop: 1 }}
/>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I avoided mapping here since the MUI Menu components don't like empty fragments and adding a Box to contain things messed up the keyboard tabbing/focus

Screenshot 2024-11-15 at 1 02 09 PM

@hana-akamai hana-akamai marked this pull request as ready for review November 15, 2024 22:03
@hana-akamai hana-akamai requested a review from a team as a code owner November 15, 2024 22:03
@hana-akamai hana-akamai requested review from dwiley-akamai and harsh-akamai and removed request for a team November 15, 2024 22:03
Copy link
Contributor

@dwiley-akamai dwiley-akamai left a comment

Choose a reason for hiding this comment

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

Unit tests pass locally & in CI ✅
Tabbing and clicking through "Create Menu" dropdown items works ✅
Code review ✅

As the viewport exceeds or falls below 1280px, the section headers lose/gain a background:

Video
Screen.Recording.2024-11-18.at.12.21.18.PM.mov

It happens in dark mode too, where it's more problematic.

The background color for the individual items being so starkly different makes this look visually imbalanced IMO:

Screenshot

Screenshot 2024-11-18 at 12 00 03 PM

@hana-akamai hana-akamai requested a review from a team as a code owner November 21, 2024 17:54
@hana-akamai hana-akamai requested review from cliu-akamai and removed request for a team November 21, 2024 17:54
Copy link

github-actions bot commented Nov 21, 2024

Coverage Report:
Base Coverage: 86.82%
Current Coverage: 87.06%

@hana-akamai
Copy link
Contributor Author

@dwiley-akamai Good catch, addressed!

Copy link
Contributor

@harsh-akamai harsh-akamai left a comment

Choose a reason for hiding this comment

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

Code review ✅
Unitt tests on Local ✅

The menu overlaps the button area when width is less than 1280px
image

@hana-akamai hana-akamai added the Add'tl Approval Needed Waiting on another approval! label Nov 22, 2024
@linode-gh-bot
Copy link
Collaborator

Cloud Manager UI test results

🔺 6 failing tests on test run #9 ↗︎

❌ Failing✅ Passing↪️ Skipped🕐 Duration
6 Failing448 Passing2 Skipped130m 56s

Details

Failing Tests
SpecTest
linode-storage.spec.tslinode storage tab » delete disk
linode-storage.spec.tslinode storage tab » add a disk
machine-image-upload.spec.tsmachine image » uploads machine image, mock finish event
machine-image-upload.spec.tsmachine image » uploads machine image, mock upload canceled failed event
machine-image-upload.spec.tsmachine image » uploads machine image, mock failed to decompress failed event
machine-image-upload.spec.tsmachine image » uploads machine image, mock expired upload event

Troubleshooting

Use this command to re-run the failing tests:

yarn cy:run -s "cypress/e2e/core/linodes/linode-storage.spec.ts,cypress/e2e/core/linodes/linode-storage.spec.ts,cypress/e2e/core/images/machine-image-upload.spec.ts,cypress/e2e/core/images/machine-image-upload.spec.ts,cypress/e2e/core/images/machine-image-upload.spec.ts,cypress/e2e/core/images/machine-image-upload.spec.ts"

@harsh-akamai harsh-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Nov 25, 2024
@hana-akamai hana-akamai merged commit f3439da into linode:develop Nov 25, 2024
22 of 23 checks passed
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! UX/UI Changes for UI/UX to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants