-
Notifications
You must be signed in to change notification settings - Fork 379
change: [UIE-8743] - Replaced Button component in DBAAS with Akamai CDS button Web Component #12148
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
base: develop
Are you sure you want to change the base?
Conversation
mockMatchMedia, | ||
renderWithTheme, | ||
getShadowRootButton, | ||
} from 'src/utilities/testHelpers'; // Import the utility |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment not needed
|
||
expect(createClusterButton).toBeInTheDocument(); | ||
const buttonHost = getByTestId('create-database-cluster'); // Query the host element |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as the getShadowRootButton
function has JS docs and comments explaining usage, we don't need the comments in the .test.tsx
getByTestId
is common so everyone knows what it's doing.
@@ -34,3 +34,9 @@ export const GroupItems = styled('ul')(({ theme }) => ({ | |||
}, | |||
padding: 0, | |||
})); | |||
|
|||
export const StyledButtonWrapper = styled('div')(({ theme }) => ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments are redundant.
@@ -58,6 +59,10 @@ interface Props { | |||
interface FormValues { | |||
configs: ConfigurationOption[]; | |||
} | |||
const StyledAddButtonWrapper = styled('div')(({ theme }) => ({ | |||
minWidth: 'auto', // Ensure no default minimum width |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Department of Redundancy Department :)
|
||
await userEvent.click(resizeButton); | ||
await userEvent.click(resizeButton as HTMLButtonElement); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious if something was complaining without the cast?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah was getting Argument of type 'HTMLButtonElement | null' is not assignable to parameter of type 'Element'.
data-qa-settings-button={buttonText} | ||
disabled={disabled} | ||
onClick={onClick} | ||
title={buttonText} | ||
title={buttonText} // Title will be passed automatically to dom since it is standard HTML attribute |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment not needed, those are internal impl. details.
hour_of_day: database.updates?.hour_of_day ?? 20, | ||
week_of_month: getInitialWeekOfMonth(), | ||
}, | ||
// validationSchema: updateDatabaseSchema, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this was already commented. Not sure why though.
* @param host - The web component host element. | ||
* @returns A promise that resolves to the button element inside the Shadow DOM, or null if not found. | ||
*/ | ||
export const getShadowRootButton = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make this function generic getShadowRoot
and then you pass in the type you're looking for? Also this needs tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides making this more generic, I'd like to find a better approach to test these components in Cloud Manager. setTimeOut
feels hacky at best and this requires to make all those tests async for just rendering DOM elements. I am hoping there's better support out there for Web Components which i'd hope could be explored before signing up on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ve generalized the utility and found a solution to eliminate the use of setTimeout. However, we still need to rely on async handling, as there’s a possibility that the Shadow DOM may not be rendered yet when attempting to access shadowRoot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new component needs to adhere to basic Accessibility standards and to be consistent with its Cloud Manager counterpart. It appears to do it right overall, but I am noticing some discrepancies and lacking a couple needed improvements:
- the focus state is barely visible when its variant is primary
- there is no active state
* @param host - The web component host element. | ||
* @returns A promise that resolves to the button element inside the Shadow DOM, or null if not found. | ||
*/ | ||
export const getShadowRootButton = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides making this more generic, I'd like to find a better approach to test these components in Cloud Manager. setTimeOut
feels hacky at best and this requires to make all those tests async for just rendering DOM elements. I am hoping there's better support out there for Web Components which i'd hope could be explored before signing up on this.
disabled={!formTouched || isSubmitting || disabled} | ||
loading={isSubmitting} | ||
processing={isSubmitting} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't pertinent to this PR as much as the component itself, but I'd argue that if the goal is to show a loading pattern, the processing
naming convention is rather unintuitive. Also, considering these components are going to live side by side with Cloud Manager components for a long time, aiming for prop consistency when possible (for which i believe it to be the case here) would go a long way. CC @jaalah-akamai
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
> | ||
Add | ||
</Button> | ||
</StyledAddButtonWrapper> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a bit confused here, why using a container to set en element's width? This points to a stylistic limitation of the component itself
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tvijay-akamai is this still needed with the changes you made to the components?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is not needed now after core component change. I have removed it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing changeset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, LGTM! I left a few nitpicks, nothing blocking, but worth considering. Also, I strongly agree with Alban's feedback. In addition, this PR needs a changeset and conflicts should be resolved before merging.
|
||
expect(createClusterButton).toBeInTheDocument(); | ||
const buttonHost = getByTestId('create-database-cluster'); // Query the host element |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Consider replacing getByTestId
with getByRole
for improved accessibility and alignment with React Testing Library best practices.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the suggestion to use getByRole for improved accessibility and alignment with React Testing Library best practices. I completely agree that getByRole is generally preferred as it aligns with accessibility standards and ensures that elements are queried based on their semantic roles.
However, in this specific case, the element we are trying to access resides inside a Shadow DOM. Unfortunately, getByRole does not penetrate the Shadow DOM or have access to elements within the Shadow Root. As a result, we need to rely on getByTestId to query the wrapper element or host element that contains the Shadow DOM. Once we have access to the wrapper, we can use a utility function like getShadowRootElement to query elements inside the Shadow DOM.
This approach ensures that we can still test Shadow DOM elements effectively while working within the constraints of the React Testing Library and the Shadow DOM API.
If there are any alternative suggestions or tools that better support Shadow DOM testing while adhering to accessibility best practices, I’d be happy to explore them further.
d3aaa19
to
bd3b76f
Compare
@abailly-akamai , please send me where i can find basic accessibility standards for cloud manager. |
@tvijay-akamai to recap, here's what we should aim to get this PR merged (spoke to Ellen about it)
|
bd3b76f
to
76e0e37
Compare
… fix the button focus styles
@abailly-akamai I have upgraded the web component library version which has the fix for the button focus styles. Please check and approve if looks good. |
Still looking into these test failures with @stayal712, but in the meantime I'm noticing a couple regressions: Button alignment on DB settings tab:
Help button alignment next to cert download button (and possibly icon color? Not sure which color is right):
Maintenance section "Upgrade Version" button alignment and appearance
|
@jdamore-linode is there an update on a solution to get the tests to pass here? |
@abailly-akamai I believe @stayal712 is working on the tests, I can follow up with her. I am interested in the recent commits to add test IDs everywhere because in my opinion that should not be necessary just to click a button. We discovered that any button that has an In the meantime, there are still unaddressed issues. |
@jdamore-linode These are addressed: #12148 (comment). Please check if looks fine. |
…equested by sakshi to add to this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to see the tests passing, thanks @stayal712! I have some comments/questions about a few of the places you're interacting with the CDS buttons
*/ | ||
findButtonByTitle: (cdsButtonTitle: string): Cypress.Chainable => { | ||
return cy | ||
.findAllByText(cdsButtonTitle) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.findAllByText(cdsButtonTitle) | |
.findByText(cdsButtonTitle) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
.then((btn) => { | ||
btn[0].click(); // Native DOM click | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stayal712 and @tvijay-akamai, do you have an explanation for why this workaround is necessary?
In my testing, I'm able to implement a simple (non-CDS) button in a shadow root, set up an event handler, and trigger it by clicking via cy.click()
. Why doesn't this work with the CDS button? Why aren't we running into any of these same issues with the text field component?
I'm willing to resort to hacky workarounds like this if they're necessary, but we should at least understand why. This is a lot of work to click on a button, and the fact that this is only an issue with the CDS button makes it sound like a bug. I want to rule that out, and I especially want to avoid implementing more hacks like this as we introduce more CDS components.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am going to try to offer a potential fix, without confidence it'll work. Bear in mind, my knowledge of web components is limited, however I'd still like to explore this before moving forward with the workaround proposed, which seem to be pointing at a potential limitation of the component.
Custom elements sometimes don’t work well with React’s synthetic event system. onClick
in React won’t fire unless the component dispatches a standard DOM event from the host. This may be the issue we've been noticing with Cypress.
Currently, this listens to @click internally on the native button, but we don't re-emit the event from the custom element. Which could perhaps be done this way:
private _handleClick(e: MouseEvent) {
if (this.type === 'submit') {
const form = this.closest('form');
if (form) {
e.preventDefault();
form.requestSubmit();
}
}
// Re-emit the click event from the custom element host
this.dispatchEvent(new MouseEvent('click', {
bubbles: true,
composed: true,
cancelable: true,
}));
}
https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/dispatchEvent
Again, not sure this will fix it, but before approving I'd like to explore this because it would also have important implications for other Web Components making their way to Cloud Manager. It could even be expanded further from what I could read, but not sure how helpful that would be.
this.dispatchEvent(new MouseEvent('click', {
bubbles: true,
composed: true,
cancelable: true,
detail: e.detail,
screenX: e.screenX,
screenY: e.screenY,
clientX: e.clientX,
clientY: e.clientY,
ctrlKey: e.ctrlKey,
shiftKey: e.shiftKey,
altKey: e.altKey,
metaKey: e.metaKey,
button: e.button,
relatedTarget: e.relatedTarget,
}));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abailly-akamai
Thank you for your suggestion. Tarun has started looking into your suggestion and will also explore alternative solutions with QA team to find the best way out to access shadow dom elements in cypress, in the next release. Meanwhile, request you to move forward with this PR for the current release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved. It would be greatly appreciated if the next PR for a web component takes in consideration all the problems raised in this one, and the compatibility with our codebase not to be an afterthought. Merging this as-is gives us no guaranty there will be a follow up. The branch cut is in a week by the way.
cy.get('[data-testid="create-database-cluster"]') | ||
.shadow() | ||
.find('button') | ||
.first() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use ui.cdsButton
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced getting shadow element with ui.cdsButton and kept .click() as a work around for this release.
cy.get('[data-testid="save-changes-button"]') | ||
.shadow() | ||
.find('button') | ||
.first() | ||
.then((btn) => { | ||
btn[0].click(); // Native DOM click | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same questions as before:
- Can we use
ui.cdsButton
here - Why isn't clicking working on submit buttons that don't have an
onClick
prop specified?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced getting shadow element with ui.cdsButton and kept .click() as a work around for this release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Echoing @abailly-akamai's thoughts but approving in the interest of keeping things moving.
I think the way these components get used in apps warrants more consideration from the CDS team; if every new CDS component is going to require wrappers, hacks, and workarounds that increase the complexity of the codebases integrating them, then this seems extremely counterproductive to the goals of the project.
@tvijay-akamai it looks like those test failures need to be addressed, cc @stayal712. It looks like you might need to narrow your selection to only click on the button you want using |
Thanks @jdamore-linode, I have made the changes to the restricted-user-details-pages.spec.ts script as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tvijay-akamai let's revisit the e2e tests
Cloud Manager UI test results🎉 605 passing tests on test run #16 ↗︎
|
Description 📝
This PR is to Replaced CM Button component in DBAAS with Akamai CDS button Web Component
Changes 🔄
Target release date 🗓️
May Release
Preview 📷
Include a screenshot or screen recording of the change.
🔒 Use the Mask Sensitive Data setting for security.
💡 Use
<video src="" />
tag when including recordings in table.How to test 🧪
Verification steps
(How to verify changes)
As an Author, to speed up the review process, I 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
As an Author, before moving this PR from Draft to Open, I confirmed ✅
Commit message and pull request title format standards
<commit type>: [JIRA-ticket-number] - <description>
Commit Types:
feat
: New feature for the user (not a part of the code, or ci, ...).fix
: Bugfix for the user (not a fix to build something, ...).change
: Modifying an existing visual UI instance. Such as a component or a feature.refactor
: Restructuring existing code without changing its external behavior or visual UI. Typically to improve readability, maintainability, and performance.test
: New tests or changes to existing tests. Does not change the production code.upcoming
: A new feature that is in progress, not visible to users yet, and usually behind a feature flag.Example:
feat: [M3-1234] - Allow user to view their login history