This repository was archived by the owner on May 24, 2024. It is now read-only.
Migrating the NotificationDialog and integrating with LayerManager #238
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tbiethman
requested review from
dkasper-was-taken,
emilyrohrbough,
mjhenkes and
ryanthemanuel
as code owners
tbiethman
commented
Jun 29, 2021
| @@ -2,14 +2,13 @@ import React from 'react'; | |||
| import PropTypes from 'prop-types'; | |||
There was a problem hiding this comment.
I recommend viewing this entire file. Git has treated this as change+move of the temporary dev-site file, but the temporary files weren't ever reviewed.
Edit: Seems I finally changed enough for git to treat this as new.
tbiethman
commented
Jun 29, 2021
| @@ -1,5 +1,5 @@ | |||
| import React, { useState } from 'react'; | |||
| import UnsavedChangesPrompt from '@cerner/terra-dev-site/lib/terra-application-temporary/unsaved-changes-prompt'; | |||
| import UnsavedChangesPrompt from '@cerner/terra-application/lib/unsaved-changes-prompt'; | |||
There was a problem hiding this comment.
Pointing to the official code now, temporary files have been removed.
mjhenkes
reviewed
Jul 1, 2021
packages/terra-application/src/notification-dialog/NotificationDialog.jsx
Outdated
Show resolved
Hide resolved
mjhenkes
reviewed
Jul 1, 2021
Comment on lines
239
to
246
| <div className={cx('overlay')} /> | ||
| <div className={cx('notification-dialog')}> | ||
| <div tabIndex="-1" ref={initialFocusAnchorRef} /> | ||
| <div className={cx('notification-dialog-inner-wrapper')}> | ||
| <div className={cx('notification-dialog-container')}> | ||
| <div className={cx('floating-header-background', variant)} /> | ||
| <div className={cx('header')}> | ||
| <div className={cx('header-content')}> |
mjhenkes
approved these changes
Jul 1, 2021
mjhenkes
approved these changes
Jul 2, 2021
…ation into story-204-notification-dialog-migration # Conflicts: # packages/terra-application-docs/src/terra-dev-site/test/terra-application/shared/TestPage.jsx # packages/terra-application/CHANGELOG.md
| * The button text and onclick values of the accept button. | ||
| */ | ||
| acceptAction: PropTypes.shape({ | ||
| text: PropTypes.string, |
There was a problem hiding this comment.
Nit. Should these sub-items be doc'd?
| * @param {Object} containerRef React ref object containing current value of | ||
| * the element to trap focus within. | ||
| */ | ||
| const useFocusTrap = (containerRef) => { |
There was a problem hiding this comment.
I like this. Good work. 👍
There was a problem hiding this comment.
Using inert everywhere allows us to keep this pretty simple. Should be able to put it in the new modals as well, I can externalize the hook when that is the case.
packages/terra-application/src/notification-dialog/NotificationDialog.jsx
Show resolved
Hide resolved
dkasper-was-taken
approved these changes
Jul 2, 2021
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Closes #204
Deployment Link
https://terra-applic-.herokuapp.com/
Testing
Additional Details
Thank you for contributing to Terra.
@cerner/terra