Skip to content

feat/1729 modal properties#145

Merged
chrisolsen merged 6 commits into
GovAlta:alphafrom
vanessatran-ddi:chore/1729-modal-properties
May 1, 2024
Merged

feat/1729 modal properties#145
chrisolsen merged 6 commits into
GovAlta:alphafrom
vanessatran-ddi:chore/1729-modal-properties

Conversation

@vanessatran-ddi
Copy link
Copy Markdown
Collaborator

@vanessatran-ddi vanessatran-ddi commented Apr 18, 2024

Description:

2 issues: 1629 and 1729

What are changed:

  • Add role attribute to Modal component. Change the example to reflect the accessibility of the modal.
  • Add arialabel attribute to Icon button sandbox, properties table and examples.
  • Fixing wrong code snippets under Modal examples, add open
  • Add one example of Change route
  • Allow sandbox renders dynamic variable like open attribute.
  • Fixing the modal overlay issue.
  • Make sure 1st toc item is always highlighted.

image

@vanessatran-ddi vanessatran-ddi self-assigned this Apr 18, 2024
@netlify
Copy link
Copy Markdown

netlify Bot commented Apr 18, 2024

Deploy Preview for abgov-ui-component-docs ready!

Name Link
🔨 Latest commit 6f0394d
🔍 Latest deploy log https://app.netlify.com/sites/abgov-ui-component-docs/deploys/6632abdd6ab1fe0009537f2f
😎 Deploy Preview https://deploy-preview-145--abgov-ui-component-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@lizhuomeng71
Copy link
Copy Markdown
Collaborator

hI, we notice something for this Warning callout modal, angular code missing an on click function
Screenshot 2024-04-23 120918
Screenshot 2024-04-23 121209

@vanessatran-ddi vanessatran-ddi force-pushed the chore/1729-modal-properties branch from 37c44a6 to aee77da Compare April 25, 2024 20:07
@vanessatran-ddi vanessatran-ddi changed the title Chore/1729 modal properties feat/1729 modal properties Apr 25, 2024
@lizhuomeng71 lizhuomeng71 marked this pull request as ready for review April 29, 2024 19:51
@lizhuomeng71
Copy link
Copy Markdown
Collaborator

Issue is verified, NVDA notice is not added to documents, probably will be handle by a sperated ticket

@vanessatran-ddi vanessatran-ddi force-pushed the chore/1729-modal-properties branch 2 times, most recently from 8fc7499 to 40a1036 Compare April 29, 2024 21:44
@vanessatran-ddi vanessatran-ddi changed the base branch from main to alpha April 29, 2024 21:50
Comment thread src/components/component-content/component-content.module.css Outdated

#dynamicProp(name: string): string {
return `[${name.toLowerCase()}]="some${this.capitalize(name)}Value"`;
return `[${name.toLowerCase()}]="${this.lowerCase(name)}"`;
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

As we agree on other pull request, this will render a dynamic variable as below:
image

@vanessatran-ddi vanessatran-ddi force-pushed the chore/1729-modal-properties branch 2 times, most recently from 7fe2567 to 6a7c3c3 Compare April 29, 2024 22:37

function isActive(id: string): boolean {
function isActive(id: string, index: number): boolean {
if (activeByScroll == null && index === 0) return true;
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This to make sure first toc item will be highlighted if there isn't anything selected.
image

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can this be moved to a separate commit?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I moved it to fcec451

Comment thread src/components/table-of-contents/toc.module.css
Lorem ipsum dolor sit amet consectetur adipisicing elit. Mollitia obcaecati id
molestiae, natus dicta, eaque qui iusto similique, libero explicabo eligendi eius
laboriosam! Repellendus ducimus officia asperiores. Eos, eius numquam.
<p>Your progress will not be saved.</p>
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Change the example so we have at least 1 element interactive/focusable.


function isActive(id: string): boolean {
function isActive(id: string, index: number): boolean {
if (activeByScroll == null && index === 0) return true;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can this be moved to a separate commit?


<ComponentProperties properties={componentProperties} />
</GoATab>
<GoATab
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can this be moved to a separate commit?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I moved it to 3b5a83e

@@ -30,6 +30,7 @@ type CastingType = {
export default function IconButtonPage() {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can this be moved to a separate commit?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I move it to 7331ade


#dynamicProp(name: string): string {
return `[${name.toLowerCase()}]="some${this.capitalize(name)}Value"`;
return `[${name.toLowerCase()}]="${name}"`;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Are all the sandbox changes for the modal, if not they should be moved to a separate commit as well.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Collaborator Author

@vanessatran-ddi vanessatran-ddi May 1, 2024

Choose a reason for hiding this comment

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

I added it under commit 5b79223

@vanessatran-ddi vanessatran-ddi force-pushed the chore/1729-modal-properties branch 3 times, most recently from fdce5ca to fc2d4d0 Compare May 1, 2024 20:49
@vanessatran-ddi vanessatran-ddi force-pushed the chore/1729-modal-properties branch from fc2d4d0 to 6f0394d Compare May 1, 2024 20:53
@vanessatran-ddi vanessatran-ddi requested a review from chrisolsen May 1, 2024 20:59
@chrisolsen chrisolsen merged commit a6e1054 into GovAlta:alpha May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Documentation: Modal Properties and Example Documentation: Modal component code

3 participants