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.

Sign up for GitHub

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

Jump to bottom

upcoming: [M3-7580] - Add search field to Create Linode UIs #10088

Merged
merged 18 commits into from
Jan 24, 2024
Merged

upcoming: [M3-7580] - Add search field to Create Linode UIs #10088

merged 18 commits into from
Jan 24, 2024

Conversation

hkhalil-akamai
Copy link
Contributor

@hkhalil-akamai hkhalil-akamai commented Jan 19, 2024 •
edited
Loading

Description 📝

Adds a search field to the Clone Linode and Create Linode from Backup flows to allow users to filter Linodes based on their labels.

These changes are gated behind the linodeCloneUIChanges feature flag.

Changes 🔄

  • Add search field to Create Linode Clone and Backup flows
  • When a user selects "Clone" on an existing Linode, navigate to Clone Linode UI, filter on the Linode's label and pre-select that Linode
  • Refactored DebouncedSearchTextField to allow field state to be externally managed
    • This was necessary to allow automatically filtering from the query param

Preview 📷

Before After
Screen.Recording.2024-01-19.at.6.30.35.PM.mov
Screen.Recording.2024-01-19.at.6.24.13.PM.mov

How to test 🧪

  1. Navigate to the Create Linode page
  2. Select the "Clone Linode"/"Backups" tab
  3. Ensure you can filter Linodes based on their label
  4. Click on a Linode's action menu and select "Clone Linode"
  5. Verify the Linode is selected in the list and the search field is pre-filled with the Linode's label
  6. Verify you can clear and edit the contents of the search field after it is pre-filled
  7. Verify existing usages of DebouncedSearchTextField continue to work as expected

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

@hkhalil-akamai hkhalil-akamai added Linodes Dealing with the Linodes section of the app Linode Clone UI Updates labels Jan 19, 2024
@hkhalil-akamai hkhalil-akamai self-assigned this Jan 19, 2024
@hkhalil-akamai hkhalil-akamai requested a review from a team as a code owner January 19, 2024 23:40
@hkhalil-akamai hkhalil-akamai requested review from jdamore-linode and carrillo-erik and removed request for a team January 19, 2024 23:40
@hkhalil-akamai hkhalil-akamai changed the title change: [M3-7580] - Add search field to Create Linode UIs added: [M3-7580] - Add search field to Create Linode UIs Jan 19, 2024
@hkhalil-akamai hkhalil-akamai changed the title added: [M3-7580] - Add search field to Create Linode UIs change: [M3-7580] - Add search field to Create Linode UIs Jan 19, 2024
hkhalil-akamai
hkhalil-akamai commented Jan 19, 2024
View reviewed changes
packages/manager/src/components/DebouncedSearchTextField/DebouncedSearchTextField.tsx
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 reworked to allow externally managing the state of the TextField. This is necessary for pre-filling the text field. This component is used in the following, which need to be tested for regressions:

  • TransferTable
  • FirewallDeviceLanding
  • FromAppsContent
  • LongviewClients
  • StackScriptBase
  • VPCSubnetsTable

hkhalil-akamai
hkhalil-akamai commented Jan 19, 2024
View reviewed changes
packages/manager/src/components/DebouncedSearchTextField/DebouncedSearchTextField.tsx
Comment on lines -47 to -55
/*
This `didCancel` business is to prevent a warning from React.
See: https://github.com/facebook/react/issues/14369#issuecomment-468267798
*/
let didCancel = false;
/*
don't run the search if the query hasn't changed.
This is mostly to prevent this effect from running on first mount
*/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to test to make sure this behavior is preserved.

hkhalil-akamai
hkhalil-akamai commented Jan 19, 2024
View reviewed changes
packages/manager/src/features/Linodes/LinodesCreate/SelectLinodePanel.tsx
<DebouncedSearchTextField
sx={{
marginBottom: theme.spacing(1),
width: '330px',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Width comes from design mockup
image

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 20, 2024 •
edited
Loading

Coverage Report: ✅
Base Coverage: 80.18%
Current Coverage: 80.18%

bnussman-akamai
bnussman-akamai reviewed Jan 23, 2024
View reviewed changes
Copy link
Member

@bnussman-akamai bnussman-akamai left a comment

Choose a reason for hiding this comment

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

Behavior looks good, just have a few comments/questions!

bnussman-akamai
bnussman-akamai reviewed Jan 23, 2024
View reviewed changes
packages/manager/src/features/Linodes/LinodesCreate/SelectLinodePanel.tsx
Comment on lines +56 to +58
const [preselectedLinodeID] = React.useState(
flags.linodeCloneUIChanges && selectedLinodeID
);
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 piece of state need to exist? Should we add a comment explaining why this is needed as opposed to using selectedLinodeID directly?

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 is to prevent the search term from changing whenever the user selects a different linode.

Good idea to include this as a comment.

packages/manager/src/components/DebouncedSearchTextField/DebouncedSearchTextField.tsx Outdated Show resolved Hide resolved
@hkhalil-akamai hkhalil-akamai changed the title change: [M3-7580] - Add search field to Create Linode UIs upcoming: [M3-7580] - Add search field to Create Linode UIs Jan 23, 2024
@carrillo-erik
Copy link
Contributor

I know you mentioned that the size of the search bar came from the mock up, however, I think it would align better with the other input fields if we match the width accordingly.

Screenshot 2024-01-23 at 12 07 33 PM

@hkhalil-akamai
Copy link
Contributor Author

hkhalil-akamai commented Jan 23, 2024 •
edited
Loading

I know you mentioned that the size of the search bar came from the mock up, however, I think it would align better with the other input fields if we match the width accordingly.

Thanks for the suggestion. I will check with UX.

@hkhalil-akamai hkhalil-akamai added Approved Multiple approvals and ready to merge! and removed Ready for Review labels Jan 24, 2024
@hkhalil-akamai hkhalil-akamai merged commit e6c5a8e into linode:develop Jan 24, 2024
18 checks passed
@cpathipa cpathipa mentioned this pull request Feb 1, 2024
Merged
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carrillo-erik carrillo-erik carrillo-erik approved these changes

@bnussman-akamai bnussman-akamai bnussman-akamai approved these changes

@jaalah-akamai jaalah-akamai Awaiting requested review from jaalah-akamai

@jdamore-linode jdamore-linode Awaiting requested review from jdamore-linode jdamore-linode is a code owner automatically assigned from linode/frontend

Assignees

@hkhalil-akamai hkhalil-akamai

Labels
Approved Multiple approvals and ready to merge! Linodes Dealing with the Linodes section of the app
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hkhalil-akamai @carrillo-erik @bnussman-akamai