feat(DeleteModal): first version #3
Conversation
adamviktora
force-pushed
the
delete-modal-tbd
branch
from
672ce79
to
5f46c9a
Compare
| } | ||
| > | ||
| <TextInput | ||
| aria-label="Delete modal input" |
There was a problem hiding this comment.
Aria labels need to be configurable for internationalization purposes
There was a problem hiding this comment.
I'd recommend using the text that displays above the text input as the aria-label for this text input. This text visibly functions as the text input label, and it would be better to reference the visible text, rather than require consumers to specify this text in 2 places.
You can do this by providing an id on the div that wraps this text, and an aria-labelledby attribute on the <TextInput> component that references the id value for the text.
But I do agree with @nicolethoen's comment above that this text needs to be configurable:
Type RedHatAwesome to confirm deletion:
| {error && ( | ||
| <Alert | ||
| data-testid="delete-model-error-message-alert" | ||
| title={`Error deleting ${itemToDelete}`} |
adamviktora
requested review from
jgiardino,
simrandhaliw and
jeff-phillips-18
adamviktora
force-pushed
the
delete-modal-tbd
branch
from
5f3639c
to
f269284
Compare
adamviktora
force-pushed
the
delete-modal-tbd
branch
from
f269284
to
3de93ef
Compare
|
These examples look great! Something I'd like for us to consider as we build out components is to make sure we aren't inadvertently promoting microcopy patterns that we don't want. For example, for the extra destructive variant includes the following message in the original content design document:
But this content pattern isn't reflected in the example. We can and should reference our design/content patterns as part of these examples under the design guidelines tab, but I'd also like to consider how content patterns are reflected in the UI component examples. Consumers are likely to assume that UI component examples embody recommended patterns and will not refer to design guidelines to confirm otherwise. One option is to use contents that are clearly placeholder text (i.e. text that describes what type of contents are recommended). Another option is to use contents that demonstrate the pattern. Or we could do a mix of both, for example:
@nicolethoen @simrandhaliw - Do you have any preferences on this? |
adamviktora
force-pushed
the
delete-modal-tbd
branch
from
6958f29
to
5abda37
Compare
| @@ -4,7 +4,7 @@ | |||
| section: extensions | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
Thanks for referencing the other lines, I tried updating this before, but didn't realize I have to change the section name in other places too
|
|
||
| ``` | ||
|
|
||
| ### Fullscreen example | ||
|
|
||
| ```js file="./Basic.tsx" isFullscreen | ||
| ```js file="./DeleteModalBasic.tsx" isFullscreen |
There was a problem hiding this comment.
I don't think need a full screen example in this case.
Some components only make sense in a full screen context. This is probably not one.
| import { ExtendedButton } from "@patternfly/ai-infra-ui-components"; | ||
| import { DeleteModal } from "@patternfly/ai-infra-ui-components"; | ||
|
|
||
| Note: this component is an upgrade of an [existing DeleteModal](https://github.com/opendatahub-io/odh-dashboard/blob/main/frontend/src/pages/projects/components/DeleteModal.tsx) in odh-dashboard | ||
|
|
||
| ## Basic usage |
There was a problem hiding this comment.
I'd recommend renaming this file to DeleteModal.md or DeleteModalExamples.md so other components can have their own markdown files in the same location.
| import { ExtendedButton } from "@patternfly/ai-infra-ui-components"; | ||
| import { DeleteModal } from "@patternfly/ai-infra-ui-components"; | ||
|
|
||
| Note: this component is an upgrade of an [existing DeleteModal](https://github.com/opendatahub-io/odh-dashboard/blob/main/frontend/src/pages/projects/components/DeleteModal.tsx) in odh-dashboard |
There was a problem hiding this comment.
I'd update this to:
Note: this component documents the API and enhances the existing DeleteModal component from odh-dashboard. It can be imported from @patternfly/ai-infra-ui-components. Alternatively, it can be used within the odh-dashboard via the import: '~/pages/projects/components/DeleteModal'
packages/module/src/index.ts
Outdated
| @@ -1 +1,2 @@ | |||
| export * from './DeleteModal'; | |||
| export * from './ExtendedButton'; | |||
There was a problem hiding this comment.
I can put up a PR to delete the extendedButton. That's what comes with the extension seed, right? so we don't need it?
There was a problem hiding this comment.
It's ok, I have deleted it as part of this PR, so we don't have to handle conflicts
|
🎉 This PR is included in version 1.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Closes #2