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

change: [M3-8377, M3-8579] - Move Region section above Images in Linode Create and update default OS to Ubuntu 24.04 LTS #10858

Merged

Conversation

hana-akamai
Copy link
Contributor

@hana-akamai hana-akamai commented Aug 29, 2024

Description 📝

  1. Move Region section above the Images section in the Linode Create flows. This change has been approved by UX/Marketing/Docs
  2. Set Default OS to Ubuntu 24.04 LTS

No changes have been made in the Backups/Clone tab since they don't have an Images section

Preview 📷

Before After
image image

How to test 🧪

Verification steps

(How to verify changes)

  • Click through the Linode Create flow tabs and ensure that the Region section is above the Images section and that the default OS is Ubuntu 24.04 LTS
  • There should be no regressions in creating a Linode

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

@hana-akamai hana-akamai added the UX/UI Changes for UI/UX to review label Aug 29, 2024
@hana-akamai hana-akamai self-assigned this Aug 29, 2024
@hana-akamai hana-akamai marked this pull request as ready for review September 9, 2024 20:42
@hana-akamai hana-akamai requested a review from a team as a code owner September 9, 2024 20:42
@hana-akamai hana-akamai requested review from bnussman-akamai and coliu-akamai and removed request for a team September 9, 2024 20:42
</Box>
</Paper>
<Stack spacing={3}>
<Region />
Copy link
Member

Choose a reason for hiding this comment

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

Now that Region is rendered in these components, it is more susceptible to re-renders if any state in this component changes.

Could we somehow place Region outside of these subcomponents. I think it would help ensure we keep Linode Create v2 performant. Also, rendering a single instance of Region at a higher level will keep Linode Create v2 easy to understand from a code perspective.

This might be tricky with how the Tabs work but maybe this is still possible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bnussman-akamai Hmm what about memoizing the Region component?

Copy link
Member

Choose a reason for hiding this comment

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

That would help, but I still think it would be great if we could keep one instance of <Region /> in the JSX so the JSX conceptually matches up with the UI

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bnussman-akamai Pushed the memo change. I tried keeping one instance of Region / having it at a higher level but since it's not always in the same place, that makes it difficult. This was the cleanest solution I could come up

Copy link
Member

@bnussman-akamai bnussman-akamai Sep 10, 2024

Choose a reason for hiding this comment

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

Ohhhh. I didn't realize this only applied to the Image tabs. My fault. I'll think if there are any other options

Copy link

github-actions bot commented Sep 10, 2024

Coverage Report:
Base Coverage: 86.93%
Current Coverage: 86.93%

@hana-akamai hana-akamai requested a review from a team as a code owner September 12, 2024 19:19
@hana-akamai hana-akamai requested review from AzureLatte and removed request for a team September 12, 2024 19:19
@hana-akamai hana-akamai changed the title change: [M3-8377] - Move Region section above Images in Linode Create change: [M3-8377, M3-8579] - Move Region section above Images in Linode Create and update default OS to Ubuntu 24.04 LTS Sep 12, 2024
@bnussman-akamai
Copy link
Member

Looks like the create-linode-mobile.spec.ts failure might be legit

@coliu-akamai
Copy link
Contributor

test failures for

  • create-linode-view-code-snippet.spec.ts (creates a linode via CLI - searched for Debian but was Ubuntu. sometimes for this test it also gets hung up on clicking the region? not too sure)
  • linode-storage.spec.ts ('try to delete in use disk' - searched for Debian 11 but was Ubuntu)

might also be related - ran them locally and the above failed

  • resize-linode.spec.ts also failed locally but not sure if that's related to these changes

Copy link
Contributor

@coliu-akamai coliu-akamai left a comment

Choose a reason for hiding this comment

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

ty Hana! 🎉
✅ confirmed region moved to top for Linode Create flow
✅ confirmed default is now Ubuntu 24.04
✅ double checked cypress tests, failing test passed locally so doesn't seem related to this pr

am curious about the ongoing thread, if there was another potential solution? 👀

@hana-akamai
Copy link
Contributor Author

hana-akamai commented Sep 20, 2024

Yeah, not sure if there's another solution that doesn't require a good amount of refactoring 😅

@hana-akamai hana-akamai added the Add'tl Approval Needed Waiting on another approval! label Sep 20, 2024
@bnussman-akamai bnussman-akamai removed the Add'tl Approval Needed Waiting on another approval! label Sep 20, 2024
@bnussman-akamai bnussman-akamai added the Approved Multiple approvals and ready to merge! label Sep 20, 2024
@hana-akamai hana-akamai merged commit 64aec6e into linode:develop Sep 20, 2024
19 of 20 checks passed
@hana-akamai hana-akamai deleted the M3-8377-move-region-to-top-create-flow branch September 20, 2024 19:29
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.

3 participants