feat: Blue dialog code snippet #2923

laveshv · 2024-10-22T20:33:42Z

Description

Related links, issue #, if available: n/a

How has this been tested?

Review checklist

The following items are to be evaluated by the author(s) and the reviewer(s).

Correctness

Changes include appropriate documentation updates.
Changes are backward-compatible if not indicated, see CONTRIBUTING.md.
Changes do not include unsupported browser features, see CONTRIBUTING.md.
Changes were manually tested for accessibility, see accessibility guidelines.

Security

If the code handles URLs: all URLs are validated through the checkSafeUrl function.

Testing

Changes are covered with new/existing unit tests?
Changes are covered with new/existing integration tests?

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

cansuaa · 2024-10-24T13:54:59Z

pages/bluedialog/bluedialog.page.tsx

+    setShowDialog(false);
+  }
+
+  return showDialog ? (


Let's not remove the dialog at all, we can console.log instead. I think overall the example here could be simpler.

Done in latest revision.

cansuaa · 2024-10-24T13:56:04Z

pages/bluedialog/bluedialog.page.tsx

+  }
+
+  return showDialog ? (
+    <div className={styles['blue-dialog-box']}>


I don't see the a11y requirements being addressed.

I added some a11y specific code. Can you please review?

cansuaa · 2024-10-29T13:30:07Z

pages/bluedialog/styles.scss

+  margin-block: 20px;
+  margin-inline: 20px;
+  border-block: 1px solid awsui.$color-border-status-info;
+  border-inline: 1px solid awsui.$color-border-status-info;


Why not just margin and border? Is margin even needed?

margin and border are disallowed in the lint settings -> https://github.com/cloudscape-design/components/blob/main/.stylelintrc#L59C39-L59C74

Actually, we can remove the margin. Just that it wasn't looking very good on my whole page. It was spreading on the whole page. So, I introduced it.

cansuaa · 2024-10-29T13:30:53Z

pages/bluedialog/styles.scss

+  padding-block: 10px;
+  padding-inline: 10px;


Refer here -> https://github.com/cloudscape-design/components/blob/main/.stylelintrc#L59C39-L59C74

cansuaa · 2024-10-29T13:33:12Z

pages/bluedialog/bluedialog.page.tsx

+      role={'dialog'}
+      aria-labelledby="dialog-title"
+      aria-modal="false" // Maintains natural focus flow since it's an inline dialog
+      tabIndex={-1} // This allows the dialog to receive focus


I believe the value should be 0 not -1

-1 means the dialog can only be focused programmatically which is good for one-time focus when it appears. I think making it user-focusable is not needed.

In either case, it would be great to show this focusing behaviour in action.

codecov · 2024-10-29T13:47:06Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.25%. Comparing base (041e7e5) to head (277560b).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2923   +/-   ##
=======================================
  Coverage   96.24%   96.25%           
=======================================
  Files         769      769           
  Lines       21722    21744   +22     
  Branches     7371     7379    +8     
=======================================
+ Hits        20907    20929   +22     
  Misses        762      762           
  Partials       53       53

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pan-kot · 2024-10-29T13:59:08Z

pages/bluedialog/bluedialog.page.tsx

+      aria-modal="false" // Maintains natural focus flow since it's an inline dialog
+      tabIndex={-1} // This allows the dialog to receive focus
+    >
+      <Form


Should we also use a semantic form element and support onSubmit?

Actually we can, but then I did not see the onSubmit event in the
component of clouscape, so wanted to navigate away from that practice. Also, it adds some over burden of styling that semantic element.

pan-kot · 2024-10-29T14:01:52Z

pages/bluedialog/bluedialog.page.tsx

+
+import styles from './styles.scss';
+
+export default function Bluedialogbox() {


Should we separate page and component?

Say, we can have some BluedialogPage that renders the screenshot area maybe and some additional heading (needed for a11y static checks anyways) etc. and the Bluedialog component that can also take some onSubmit/onCancel props and maybe some other, showcasing the actual design of the component?

Actually, the approach that we decided was that we will not create any built-in component for this request. This whole tasks would be to just create a code snippet that the users can copy and use, with minimum changes from their end.

Also, I think this folder, might not be the right place for this code. So, please feel free to suggest if there is a better place to put these files

I believe Andrei's suggesting to do it this way so that customers would directly be able to copy the "component". Similarly the reason why he suggested to put the functions like onCancel outside of the "component"

pan-kot · 2024-10-29T14:05:24Z

pages/bluedialog/bluedialog.page.tsx

+
+  useEffect(() => {
+    // Store the element that had focus before dialog opened
+    triggerRef.current = document.activeElement as HTMLElement;


If the dialog is opened programmatically this transition can be weird. Would it be better to handle the focus transition on the outside? If the parent component provides onSubmit/onCancel then the focus transition can be done as part of it:

function ParentComponent() { // ... function returnFocus() { sendButtonRef.current?.focus() } function onSubmit() { // submit returnFocus(); } function onCancel() { // cancel returnFocus(); } return ( <div> // ... <Bluedialog onSubmit={onSubmit} onCancel={onCancel} /> </div> ); }

Like I mentioned previously, this is just a code snippet. It is upto the individual users on how they want to use it.

laveshv · 2024-10-30T00:52:53Z

I refactored this little code little bit. Converted BluedialogPage into a parent page and move all its code to child component dialog

laveshv requested a review from a team as a code owner October 22, 2024 20:33

laveshv requested review from YueyingLu and removed request for a team October 22, 2024 20:33

cansuaa reviewed Oct 24, 2024

View reviewed changes

cansuaa changed the title ~~Bluedialog Initial commit~~ feat: Blue dialog code snippet Oct 24, 2024

laveshv force-pushed the main branch from 0bb9381 to f22a2c2 Compare October 26, 2024 00:27

cansuaa reviewed Oct 29, 2024

View reviewed changes

laveshv had a problem deploying to dev-pages October 29, 2024 13:47 — with GitHub Actions Failure

cansuaa deployed to dev-pages October 29, 2024 13:49 — with GitHub Actions View deployment

pan-kot reviewed Oct 29, 2024

View reviewed changes

cansuaa marked this pull request as draft October 29, 2024 14:11

cansuaa removed the request for review from YueyingLu October 29, 2024 14:11

laveshv marked this pull request as ready for review October 30, 2024 21:13

laveshv added 6 commits October 30, 2024 14:13

Create bluedialog.page.tsx

6b7f144

Create styles.scss

af0b437

Update styles.scss

c52b104

some a11y changes and fixing style lint issues

776c3e7

Changes related to focus management

651f99f

Adding a parent page that will consume this bluedialog component

28a4770

laveshv force-pushed the main branch from 5b463f8 to 28a4770 Compare October 30, 2024 21:13

Merge branch 'main' into main

8b8dac3

cansuaa marked this pull request as draft November 4, 2024 13:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Blue dialog code snippet #2923

feat: Blue dialog code snippet #2923

laveshv commented Oct 22, 2024

cansuaa Oct 24, 2024

laveshv Oct 26, 2024

cansuaa Oct 24, 2024

laveshv Oct 28, 2024

cansuaa Oct 29, 2024

laveshv Oct 29, 2024

cansuaa Oct 29, 2024

laveshv Oct 29, 2024

cansuaa Oct 29, 2024

pan-kot Oct 29, 2024

pan-kot Oct 29, 2024

codecov bot commented Oct 29, 2024 •

edited

Loading

pan-kot Oct 29, 2024

laveshv Oct 29, 2024

pan-kot Oct 29, 2024

laveshv Oct 29, 2024 •

edited

Loading

cansuaa Nov 4, 2024 •

edited

Loading

pan-kot Oct 29, 2024

laveshv Oct 29, 2024

laveshv commented Oct 30, 2024


		import styles from './styles.scss';

		export default function Bluedialogbox() {

feat: Blue dialog code snippet #2923

Are you sure you want to change the base?

feat: Blue dialog code snippet #2923

Conversation

laveshv commented Oct 22, 2024

Description

How has this been tested?

Correctness

Security

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 29, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laveshv Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

cansuaa Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laveshv commented Oct 30, 2024

codecov bot commented Oct 29, 2024 •

edited

Loading

laveshv Oct 29, 2024 •

edited

Loading

cansuaa Nov 4, 2024 •

edited

Loading