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-7700] - Make ACLB Copy Changes Requested by UX #10128

Merged
merged 16 commits into from
Jan 31, 2024
Merged

upcoming: [M3-7700] - Make ACLB Copy Changes Requested by UX #10128

merged 16 commits into from
Jan 31, 2024

Conversation

bnussman-akamai
Copy link
Member

@bnussman-akamai bnussman-akamai commented Jan 30, 2024 •
edited
Loading

Description 📝

  • Makes various copy changes requested of UX for ACLB
    • Look in the internal ticket (M3-7700) for a link to the copy provided by UX

How to test 🧪

Prerequisites

  • Use the MSW or use the dev-test-aglb account with the dev environment

Verification steps

  • Go through the doc on the internal ticket (M3-7700) and verify all copy changes have been implemented.

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

@bnussman-akamai bnussman-akamai added Work in Progress ACLB Relating to the Akamai Cloud Load Balancer labels Jan 30, 2024
@bnussman-akamai bnussman-akamai self-assigned this Jan 30, 2024
bnussman-akamai
bnussman-akamai commented Jan 30, 2024
View reviewed changes
packages/manager/src/features/LoadBalancers/LoadBalancerDetail/ServiceTargets/constants.tsx
Comment on lines -42 to -43
{ label: 'TCP', value: 'tcp' },
{ label: 'HTTP', value: 'http' },
Copy link
Member Author

Choose a reason for hiding this comment

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

UX wanted the order changed so that HTTPS was first

@bnussman-akamai bnussman-akamai marked this pull request as ready for review January 30, 2024 20:47
@bnussman-akamai bnussman-akamai requested a review from a team as a code owner January 30, 2024 20:47
@bnussman-akamai bnussman-akamai requested review from hana-akamai and hkhalil-akamai and removed request for a team January 30, 2024 20:47
@github-actions GitHub Actions
Copy link

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

Coverage Report: ✅
Base Coverage: 81.15%
Current Coverage: 81.15%

hkhalil-akamai
hkhalil-akamai reviewed Jan 30, 2024
View reviewed changes
Copy link
Contributor

@hkhalil-akamai hkhalil-akamai left a comment

Choose a reason for hiding this comment

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

Noticing a few differences from the document:

  • Mentions a field called "Health Check Host Header" that doesn't exist in ServiceTargetDrawer
  • Mentions a field called "Hostname Match (optional) that is currently called "Hostname (optional) in RuleDrawer and also has different tooltip content.
  • Describes an "Algorithm tooltip" that is displayed as a description in ServiceTargetDrawer

packages/manager/src/features/LoadBalancers/LoadBalancerDetail/Routes/constants.ts Outdated
@@ -1,5 +1,7 @@
export const ROUTE_COPY = {
Description: {
Description:
'Routes assigned to a load balancer’s configuration, define how incoming requests are directed to service targets.',
Copy link
Contributor

Choose a reason for hiding this comment

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

This is identical to the UI copy, but the comma seems unnecessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed 087e58c

@bnussman-akamai bnussman-akamai requested a review from a team as a code owner January 31, 2024 04:45
@bnussman-akamai bnussman-akamai requested review from jdamore-linode and removed request for a team January 31, 2024 04:45
@bnussman-akamai
Copy link
Member Author

@hkhalil-akamai

  1. Health Check Host Header should be in the Service Target Drawer now
  2. Hostname Match should now show in the Rule Drawer
  3. The doc might not reflect it, but UX wants the Algorithm helper text moved out of the tooltip and into the drawer itself. No more tooltip.

hkhalil-akamai
hkhalil-akamai approved these changes Jan 31, 2024
View reviewed changes
Copy link
Contributor

@hkhalil-akamai hkhalil-akamai left a comment

Choose a reason for hiding this comment

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

Thanks for making requested changes 🚀

mjac0bs
mjac0bs approved these changes Jan 31, 2024
View reviewed changes
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.

Everything looked like it matched the doc, though I was a little confused by the TLS Certificates / Add Certificate note. Are we adding the Host Header field to the Upload TLS Certificate drawer later?

image

Also had one copy suggestion because the sentence didn't read smoothly as is.

packages/manager/src/features/LoadBalancers/LoadBalancerDetail/Certificates/constants.tsx Outdated Show resolved Hide resolved
@mjac0bs mjac0bs added Approved Multiple approvals and ready to merge! and removed Ready for Review labels Jan 31, 2024
bnussman and others added 2 commits January 31, 2024 13:21
@bnussman
use correct docs link urls
2253a5d
@bnussman-akamai @mjac0bs
Update packages/manager/src/features/LoadBalancers/LoadBalancerDetail…
d58ede2 
…/Certificates/constants.tsx

Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com>
@bnussman-akamai
Copy link
Member Author

@mjac0bs I think you're thinking of this drawer on the configurations tab.

Screenshot 2024-01-31 at 1 27 09 PM

@mjac0bs
Copy link
Contributor

mjac0bs commented Jan 31, 2024

@bnussman-akamai Oops, yep, thanks. I looked in the Configurations tab, but must have only looked at existing configurations that were not HTTPS because I didn't see that button for the Cert drawer and got confused.

@cpathipa
Copy link
Contributor

Merging this PR on behalf of @bnussman

@cpathipa cpathipa merged commit ca489da into linode:develop Jan 31, 2024
17 of 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

@hkhalil-akamai hkhalil-akamai hkhalil-akamai approved these changes

@cpathipa cpathipa cpathipa approved these changes

@mjac0bs mjac0bs mjac0bs approved these changes

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

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

Assignees

@bnussman-akamai bnussman-akamai

Labels
ACLB Relating to the Akamai Cloud Load Balancer Approved Multiple approvals and ready to merge!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bnussman-akamai @mjac0bs @cpathipa @hkhalil-akamai @bnussman