Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(ui): Make the delete confirmation dialog less verbose #1924

Merged
merged 2 commits into from
Feb 6, 2025

Conversation

sschuberth
Copy link
Contributor

No description provided.

Copy link
Contributor

@Etsija Etsija left a comment

Choose a reason for hiding this comment

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

The DeleteDialog component has had a DOM nesting bug from the start, which I'm bringing up here, as you are anyway modifying the component: AlertDialogDescription is specified as a HTML paragraph element, which according to the HTML standards cannot contain any block elements inside it. Therefore, any time a DeleteDialog is opened in the UI, browser throws an error, which you can see in the Web Developer Tool's console:

In HTML, <div> cannot be a descendant of <p>.
This will cause a hydration error.

To fix the errors, one would need to find a way to represent the contents inside AlertDialogDescription without using any block elements like <div> or <p> (<span> is an inline element, which is allowed in this case). The best way I think is to not have one description, but several (as it's quite allowed). This refactoring gets rid of the DOM nesting errors, as you can see, now the description components do not contain any HTML block elements:

        <AlertDialogHeader>
          <div className='flex items-center'>
            <OctagonAlert className='h-8 w-8 pr-2 text-red-500' />
            <AlertDialogTitle>Confirm deletion</AlertDialogTitle>
          </div>
        </AlertDialogHeader>
        <div className='flex flex-col gap-2'>
          <AlertDialogDescription>
            Note that deletion is irreversible and might have unwanted side
            effects.
          </AlertDialogDescription>
          <AlertDialogDescription>
            If you are sure to delete the {thingName}{' '}
            <span className='font-bold'>{thingId}</span>, enter the bold text
            below for confirmation.
          </AlertDialogDescription>
          <AlertDialogDescription>
            <Input
              autoFocus
              value={input}
              onChange={(e) => setInput(e.target.value)}
            />
          </AlertDialogDescription>
        </div>

Maybe put that rearrangement of the DOM into a separate fix commit?

Signed-off-by: Sebastian Schuberth <sebastian@doubleopen.org>
@Etsija Etsija force-pushed the delete-less-verbose branch from f4bb52d to 6046acc Compare February 6, 2025 07:46
@Etsija Etsija enabled auto-merge February 6, 2025 08:09
ui/src/components/delete-dialog.tsx Outdated Show resolved Hide resolved
ui/src/components/delete-dialog.tsx Outdated Show resolved Hide resolved
@Etsija Etsija force-pushed the delete-less-verbose branch from 6046acc to ecb98b2 Compare February 6, 2025 09:43
@Etsija Etsija requested a review from lamppu February 6, 2025 09:44
@Etsija Etsija force-pushed the delete-less-verbose branch 2 times, most recently from 1d41465 to cc14c5d Compare February 6, 2025 10:03
Since AlertDialogDescription is specified as a HTML paragraph element, it
cannot contain any block elements inside it [1], otherwise it will cause
hydration errors in the browser. Fix this by rearranging the component to
have several AlertDialogDescription components, each with no block
elements inside.

[1]: shadcn-ui/ui#693 (comment)

Signed-off-by: Jyrki Keisala <jyrki.keisala@doubleopen.org>
@Etsija Etsija added this pull request to the merge queue Feb 6, 2025
Merged via the queue into main with commit c8c56a9 Feb 6, 2025
27 checks passed
@Etsija Etsija deleted the delete-less-verbose branch February 6, 2025 10:48
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.

3 participants