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.40%. Comparing base (b3e4bf7) to head (793d280).
Report is 8 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2923   +/-   ##
=======================================
  Coverage   96.40%   96.40%           
=======================================
  Files         784      784           
  Lines       22136    22138    +2     
  Branches     7190     7190           
=======================================
+ Hits        21341    21343    +2     
  Misses        788      788           
  Partials        7        7

☔ 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 · 2024-12-20T02:06:05Z

Attching screenshot for reference

cansuaa · 2024-12-24T09:41:59Z

pages/bluedialog/dialog.tsx

+  const [feedbackOptions, setFeedbackOptions] = React.useState({
+    harmful: false,
+    incomplete: false,
+    inaccurate: false,
+    other: false,
+  });


Instead of this you can have selectedFeedbackOption where you set the value to one of harmful, incomplete, etc. That way you won't need to spread objects during state setting below and it'll be easier to understand. What do you think?

I think this needs to be a multi select, where one or more options can be selected at a time. Hence, I chose to maintain a separate property for each option.

Oh right multi choice makes sense in this case.

cansuaa · 2024-12-24T09:45:04Z

pages/bluedialog/dialog.tsx

+  function submitData() {
+    onSubmit({ feedbackOptions, feedbackText });
+  }


Maybe we could add validation here to ensure one of the checkboxes are selected before submission

I was not aware of this use case. Hence avoided it to leave it on the user's choice. I will implement it in the new revision.

Actually, I am not sure how we want to handle it. This needs to be discussed if there can be a use case where we don't want to select any option, but just want to submit the 'Tell us more' section. Will implement it after discussion.

But "Tell us more" is optional. I believe at least one of the boxes should be checked but we can discuss it.

cansuaa · 2024-12-24T09:47:15Z

pages/bluedialog/dialog.tsx

+    <div
+      className={styles['blue-dialog-box']}
+      role={'dialog'}
+      aria-labelledby="dialog-title"


I don't see a corresponding element to dialog-title? Instead we could use aria-label and set it to "Feedback dialog"?

Yes, I think I forgot to edit it, from previous suggested change. Made the change in new revision.

cansuaa · 2024-12-24T09:48:56Z

pages/bluedialog/dialog.tsx

+      tabIndex={-1} // This allows the dialog to receive focus
+    >
+      <Form>
+        <div className={styles['blue-dialog-box__content']}>


Let's use Box instead
https://cloudscape.design/components/box/?tabId=playground&example=margins-and-paddings---one-side

I thought of using Box, but doesn't allow me add css class to it. It gives me more control to use div here. I also see this practice in other components as well.

cansuaa · 2024-12-24T09:50:15Z

pages/bluedialog/dialog.tsx

+      <Form>
+        <div className={styles['blue-dialog-box__content']}>
+          <SpaceBetween direction="vertical" size="l">
+            <FormField label="What did you dislike about the response?">


We still need to show a dismiss button

Done in the new Revision. Thanks for pointing it out

laveshv

Addressed comments on previous revision

cansuaa · 2025-01-07T13:27:33Z

pages/bluedialog/dialog.tsx

+    <div
+      className={styles['blue-dialog-box']}
+      role={'dialog'}
+      aria-labelledby="feedback dialog"


feedback-dialog is not defined anywhere, could you replace this with aria-label?

cansuaa · 2025-01-07T13:32:39Z

pages/bluedialog/dialog.tsx

+  const [feedbackOptions, setFeedbackOptions] = React.useState({
+    harmful: false,
+    incomplete: false,
+    inaccurate: false,
+    other: false,
+  });


Oh right multi choice makes sense in this case.

pan-kot · 2025-01-07T14:07:41Z

pages/bluedialog/dialog.tsx

+
+    // Focus the dialog container when it opens
+    if (dialogRef.current) {
+      dialogRef.current.focus();


The dialog ref is not assigned anywhere so this code does nothing.

pan-kot · 2025-01-07T14:10:28Z

pages/bluedialog/dialog.tsx

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


Would it make more sense to handle the focus management from the outside using the onSkip? Otherwise there are corner cases like if the trigger disappears after opening the dialog etc. that are not covered by this approach. Or, if the dialog opens after an async transition or automatically - the last focused element might not be related to the dialog at all.

pan-kot · 2025-01-07T14:13:27Z

pages/bluedialog/styles.scss

+
+.blue-dialog-box__footer {
+  border-block-start: 1px solid awsui.$color-border-divider-default;
+  padding-block: 10px;


can we use design tokens here?

pan-kot · 2025-01-07T14:13:50Z

pages/bluedialog/styles.scss

+  border-block: 1px solid awsui.$color-border-status-info;
+  border-inline: 1px solid awsui.$color-border-status-info;
+  background-color: awsui.$color-background-status-info;
+  border-start-start-radius: 16px;


can we use $border-radius-container here?

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 force-pushed the main branch from 5b463f8 to 28a4770 Compare October 30, 2024 21:13

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

laveshv marked this pull request as ready for review December 20, 2024 01:46

laveshv added 9 commits December 23, 2024 11:22

Create bluedialog.page.tsx

4de26b6

Create styles.scss

e3ab331

Update styles.scss

23677fb

some a11y changes and fixing style lint issues

e78ac03

Changes related to focus management

e099eaa

Adding a parent page that will consume this bluedialog component

8436467

Changing the input fields

3c3014f

Increasing the distance between the options

eb3e450

CSS changes related to footer and stretching textarea

7d474df

laveshv force-pushed the main branch from 3abd8c0 to 7d474df Compare December 23, 2024 19:22

laveshv requested a review from cansuaa December 23, 2024 21:40

cansuaa marked this pull request as draft December 24, 2024 09:37

laveshv had a problem deploying to dev-pages December 24, 2024 09:43 — with GitHub Actions Failure

cansuaa reviewed Dec 24, 2024

View reviewed changes

Addressed comments on PR

793d280

laveshv commented Jan 7, 2025

View reviewed changes

laveshv marked this pull request as ready for review January 7, 2025 05:36

cansuaa deployed to dev-pages January 7, 2025 13:31 — with GitHub Actions View deployment

cansuaa reviewed Jan 7, 2025

View reviewed changes

pan-kot reviewed Jan 7, 2025

View reviewed changes


		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

laveshv commented Dec 20, 2024

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

laveshv Jan 7, 2025 • edited Loading

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

laveshv Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laveshv left a 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

laveshv Oct 29, 2024 •

edited

Loading

cansuaa Nov 4, 2024 •

edited

Loading

laveshv Jan 7, 2025 •

edited

Loading

laveshv Jan 6, 2025 •

edited

Loading