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

Feature/242 a11y modal #657

Merged
merged 13 commits into from
Nov 16, 2022
Merged

Feature/242 a11y modal #657

merged 13 commits into from
Nov 16, 2022

Conversation

AndersonTarren
Copy link
Contributor

@AndersonTarren AndersonTarren commented Nov 4, 2022

Critical Changes

Changes

Converts existing service schedule wizard modals to lightning-modal to better support accessibility.

Issues Closed

New Metadata

Deleted Metadata

Definition of Done

Refer to Asteroids DoD document to see any additional details for the items below

  • Any net new LWC work has Sa11y tests & 50% or above lines JEST test coverage
  • CRUD/FLS is enforced in Apex
  • Permission sets are updated to account for CRUD|FLS|Tab|Classes
  • Field sets are updated to account for new fields
  • UX approval or UX not necessary
  • Link the pull request and work item by PR comment and Chatter post respectively, e.g. GUS: W-0000000: Work Name (Reorganize code; use custom iterator #303)
  • All acceptance criteria have been met
    • Developer
    • Code Reviewer
    • QA
  • PR contains draft release notes
  • QE story level testing completed

@AndersonTarren
Copy link
Contributor Author

W-11550869

Comment on lines -24 to -39
it("modal appears in non-experience-cloud context and element is accessible", () => {
element.isCommunity = false;
document.body.appendChild(element);

return global.flushPromises().then(async () => {
const modal = element.shadowRoot.querySelector("c-modal");

// Modal will only display with a spinner loaded
expect(modal).not.toBeNull();
modal.dispatchEvent(new CustomEvent("dialogclose"));

// TODO: Validate accessibility when each step is loads.
global.isAccessible(element);
});
});

Copy link
Contributor Author

@AndersonTarren AndersonTarren Nov 9, 2022

Choose a reason for hiding this comment

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

We'll have to replace this test using the new modal functionality. It's unclear in the short term how this is done - more research required.

I did add some of the test components required to make the existing test pass, we'll leverage those in the new test as well.

Comment on lines +1113 to +1119
<labels>
<fullName>Service_Schedule_Wizard</fullName>
<language>en_US</language>
<protected>true</protected>
<shortDescription>Service Schedule Wizard</shortDescription>
<value>Service Schedule Wizard</value>
</labels>
Copy link
Contributor Author

@AndersonTarren AndersonTarren Nov 9, 2022

Choose a reason for hiding this comment

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

@fanwar-salesforce This is the label used in the description for the wizard modal. I kept it very generic since it is used in a variety of situations, but not displayed on screen.

@AndersonTarren
Copy link
Contributor Author

Note to reviewer: I kept the experience cloud functionality as-is. As demonstrated in an earlier call, we're seeing an issue where exp cloud displays an extra empty modal using the new component. We'll create a new WI to investigate if this is a core issue.

@jjbennett jjbennett added in dev review Review is in progress and removed ready for review labels Nov 10, 2022
this[NavigationMixin.Navigate]({
type: "standard__objectPage",
attributes: {
objectApiName: prefixNamespace(SERVICE_SCHEDULE),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jjbennett It would have been better to just use the object schema import here and call this with objectApiName: SERVICE_SCHEDULE.objectApiName

?

Copy link
Contributor

Choose a reason for hiding this comment

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

yea probably

Copy link
Contributor

@jjbennett jjbennett left a comment

Choose a reason for hiding this comment

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

Code looks good, I am wondering about passing down the serviceId and a couple other variables you have. I did some testing and it's definitely not setting the serviceId but I am now running install prod to see if that was introduced here or elsewhere.

@api content;
serviceId;
recordTypeId;
ssCreator;
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this for?

this[NavigationMixin.Navigate]({
type: "standard__objectPage",
attributes: {
objectApiName: prefixNamespace(SERVICE_SCHEDULE),
Copy link
Contributor

Choose a reason for hiding this comment

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

yea probably

@bmuniswamy bmuniswamy merged commit c77f685 into feature/242 Nov 16, 2022
@bmuniswamy bmuniswamy deleted the feature/242__a11y-modal branch November 16, 2022 20:45
@salesforce-org-metaci salesforce-org-metaci bot mentioned this pull request Nov 16, 2022
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in dev review Review is in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants