Hi @CCiufo-WMF, thank you for your prompt response! I truly appreciate both design and code reviews. Your feedback and suggestions on how I utilize Codex would be extremely helpful. I am eagerly anticipating your review.

Hi @Dogu - first off, I just wanted to say: wow! This is a set of extensive UIs with complex forms, and it looks awesome and works really well. It was exciting to see so many Codex components in use, especially components that haven't been used much yet, like ChipInput. I showed the rest of the Design Systems Team the UIs and everyone was super impressed.

I also greatly appreciated the clear setup and testing documentation for the extension, which got me up and running quickly - thank you!

Most of my recommendations relate to improving web accessibility through changing the markup that is output by the Vue components. I don't have many design comments; the dialogs in particular look great and have a smooth UX.

Please let me know if you have any questions or would like to discuss any of these items further. We'd also love to hear any feedback you have about using Codex (the components, documentation, etc.), either now or in the future. If you end up using the design tokens at some point, or doing more work with Codex, please let us know if you run into any issues or have questions. You can always reach us on the DST Talk page or open a task on the Codex board for bug reports or feature requests.

General comments

1. Instead of listing the entire @wikimedia/codex module as a dependency of of the ext.adiutor module, you can use our new "code splitting" feature to load only the components you actually use to improve performance. Please reference the Subset of components section of the Codex page for instructions.
2. I would recommend changing the ID of the element to which you mount the adiutorSettings component from adiutor-settings-teleport-target to adiutor-settings-app. teleport-target is meant to be specific to components that use Vue's <teleport> feature, like dialogs, so I think the current ID of that element could be confusing.
3. There are a few places where a hardcoded string is provided as a prop, but a translatable message should be provided instead:
   • The Dialog component's closeButtonLabel prop (found in all of the dialog components)
   • The Message component's dismissButtonLabel prop (found in createSpeedyDeletion and the option components. Note that I did see an i18n message for this in deletionProposeOptions)
   • The ChipInput component's removeButtonLabel prop
   • The aria-label attributes on TextInputs used within tables in the option components (found in articleTaggingOptions, reportRevisionOptions, and requestPageProtectionOptions) and articleTagging

Styles

1. If you use Less for all of your style blocks, you could make use of Codex design tokens. This would enable you to use standard variables for things like colors, font styles, spacing, and more. This can help you create a more cohesive, consistent design without needing to think about it very much, since most of the design decisions are captured in the token values. Check out the Using Codex design tokens section on the Codex page for info on how to use tokens in MediaWiki code.
2. I would recommend adding an extension-specific prefix to your CSS classes to avoid accidental style collisions with other styles on the page, especially for classes like .header and .footer. See the MediaWiki CSS code conventions for more info.
3. I would also recommend scoping all styles on Codex class selectors to a feature-specific class. For example, instead of targeting .cdx-toggle-switch, target .ext-adiutor-options .cdx-toggle-switch to ensure that you're only styling the ToggleSwitches in your feature.
4. In the 6 dialog components, there are a few invalid selectors causing styles not to be applied. However, I actually think that the UI looks correct without these styles, so these blocks should just be removed. They are:
   • .csd-dialog .cdx-dialog (this is from createSpeedyDeletion.vue, but the other dialogs have a similar selector). These two selectors are on the same element, so this selector is invalid
   • .cdx-dialog cdx-label - the second class is missing its .
5. The 6 dialog components place styles on the .cdx-dialog__header element, but many of them are duplicated from styles from the Codex Dialog component itself. I think all of these styles except perhaps the padding can be deleted. That said...
6. If you can avoid it, I would recommend against adding special styles on the header and footer of the dialog that make them look different than the standard Codex Dialog, e.g. changing the font size of the h2 or the padding of the header. The Dialog is a system component meant to help users complete important actions, so it's better if it looks consistent across all features.

General Field comments

1. There are some instances, especially in the Options components, where the Field component is being used as a wrapper/structural component. I would recommend against this, and instead that you do the following:
   • For groups of related fields, i.e. fields that do not make sense outside of the group, use the field component with isFieldset set to true. This will output a <fieldset> element, helping assistive technology identify the inputs as a group. For Adiutor, I think this will only apply to groups of Checkboxes or Radios, Checkboxes with sub-items, and maybe one instance of a group of Selects in requestPageProtection. Of course, not all groups of Checkboxes are necessarily related - for example, the two checkboxes in the footer of createSpeedyDeletion are not related, and are rightly not grouped in a fieldset.
   • All other fields should contain a single input and not have is-fieldset="true". I don't think there are any circumstances where you should be nesting <cdx-field> components except in articleTagging.
2. You should use the Field component's label slot instead of the Label component. This is especially important when isFieldset is true, because a <legend> element will be output, which is important for accessibility. In most circumstances (with one exception in createSpeedyDeletion below), you can use the #label slot and you can get rid of the <strong> tags because labels are bold by default (unless they're part of a nested field, which you shouldn't need anymore)

So this:

<cdx-label class="adt-label">
    <strong>{{ namespaceReason.name }}</strong>
</cdx-label>

Would change to this:

<template #label>
    {{ namespaceReason.name }}
</template>

3. If you have a visible label, you do not also need an aria-label. You should continue to use it when there is no visible label though (like in the options tables)

Component-specific comments

articleTagging

1. For the TextInputs that appear when a specific box is checked: don't just use placeholders, which are not fully accessible, but use aria-labels as well
2. I'd recommend adding a margin-left to sub-fields that appear when a specific box is checked. That will help visually group the sub-items with the parent item. This is especially important when there are a lot of sub-item checkboxes.
3. I'd consider adding a margin-bottom to TextInputs when they appear as sub-items to separate them (like for "Article needs attention from an expert on the subject")

createSpeedyDeletion

1. Use the #label slot for the fieldsets instead of the Label component
2. You should not wrap the adiutor-warning message in a separate fieldset. I'd recommend either just removing the <cdx-field> tag wrapping it, or using the Field component's status/messages props. You could do the latter like this:

<cdx-field
    v-for="namespaceReason in namespaceDeletionReasons"
    :is-fieldset="true"
    :status="namespaceDeletionReasons.length ? 'warning' : 'default'"
    :messages="{ warning: $i18n( 'adiutor-no-namespace-reason-for-csd-title' ) }"
>

3. The copyVioInput TextInput has an aria-label from the Codex demos that should be removed

deletionPropose

1. Set isFieldset to true for the Radio group and use the #label slot
2. The TextArea should be a separate field from the Radio group; use the #label slot
3. The Button in the footer doesn't need an aria-label since it has a visible label

reportRevision

1. Remove the 2 outermost <cdx-field> elements that wrap all the fields
2. Move the "Rationale" label to the Radio fieldset using the #label slot
3. Don't set isFieldset to true for the TextArea, and use the #label slot

requestPageMove

1. The TextInput and TextArea should be separate Fields and should both use the #label slot
2. You don't need the aria-label on the TextInput since you've got a visible label

requestPageProtection

1. Remove the outermost <cdx-field> element that wraps all the fields
2. If you want the two Selects to be grouped, you should set isFieldset to true for the Field that wraps them, and use the #label slot so you get the <legend> element
3. I'd recommend making the TextArea a separate field with the "Rationale" label

All Options components

1. You might consider wrapping the entire form in a <form> element, including the "save" button at the top. This will give the button type="submit" which could be helpful to users of assistive technology.
2. I'd recommend using h2s instead of h3s for the tab headings. This will maintain the proper heading hierarchy on the page (right now there is no h2)
3. Don't wrap the top section with the heading, description, button, etc. in a Field component. Same with the sections of the form - these shouldn't be wrapped in the Field component. I'd recommend just using a <div> for separate parts of the form. This includes the table sections.
4. Make sure you're following the guidelines for fieldsets vs. standalone fields listed above. isFieldset should be true for input groups like checkbox and radio groups, but nothing else. For example, in articleTaggingOptions, there is a ToggleSwitch wrapped in its own Field component with is-fieldset="true" - the prop should be removed since it's just a single input.
5. Use the #label slot instead of the Label component
6. There are a bunch of TextInputs and TextAreas that have both a Label component and an aria-label I'd recommend just using the #label slot and removing the aria-label. The TextInputs within the tables are an exception - if an individual TextInput has no visible label, it should have an aria-label (which you've already done!)
7. For the ToggleSwitches, you can add :align-switch="true" as a prop to ensure that the label is aligned to the left and the switch is aligned to the right. Then you can remove the styles you wrote for .cdx-toggle-switch
8. Consider making the "Summaries" line an h3 and not wrapping this section in a field. Each item under Summaries can be an individual field, not nest fields or fieldsets

Again, let me know if you want to discuss anything further, or if there's anything I'm not understanding properly! Happy to chat about it in the comments here.

Hello @AnneT, first of all, thank you very much for your valuable comments and insights. I really appreciate your acknowledgment of my efforts. Based on your feedbacks, I have prepared a patch that I will push to Gerrit, and I'll add you as a reviewer. I just wanted to ask if there are any obstacles to the deployment of the extension by WMF at the moment. Can I address other minor things in subsequent updates?

Change 995016 had a related patch set uploaded (by Abaris; author: Abaris):

[mediawiki/extensions/Adiutor@master] Refine Codex usages and Vue components

https://gerrit.wikimedia.org/r/995016

In T355358#9507878, @Dogu wrote:

Hello @AnneT, first of all, thank you very much for your valuable comments and insights. I really appreciate your acknowledgment of my efforts. Based on your feedbacks, I have prepared a patch that I will push to Gerrit, and I'll add you as a reviewer. I just wanted to ask if there are any obstacles to the deployment of the extension by WMF at the moment. Can I address other minor things in subsequent updates?

Great, I just submitted some comments on your patch. Yes, I think other minor things can be addressed in future patches–I don't see any of the remaining UI improvements as a blocker to deployment. I see that you've applied for a security review and created tasks on the extension setup and review boards, so it looks like you're on the right track.

@AnneT I'm glad to have heard this, thank you for taking the time and showing interest.

Change 995016 merged by Abaris:

[mediawiki/extensions/Adiutor@master] Refine Codex usages and Vue components

https://gerrit.wikimedia.org/r/995016

Change 995351 had a related patch set uploaded (by Abaris; author: Abaris):

[mediawiki/extensions/Adiutor@master] Refine Codex usages and Vue components

https://gerrit.wikimedia.org/r/995351

Change 995351 merged by Abaris:

[mediawiki/extensions/Adiutor@master] Refine Codex usages and Vue components

https://gerrit.wikimedia.org/r/995351

Change 995355 had a related patch set uploaded (by Abaris; author: Abaris):

[mediawiki/extensions/Adiutor@master] Refine Codex usages and Vue components

https://gerrit.wikimedia.org/r/995355

Change 995355 merged by Abaris:

[mediawiki/extensions/Adiutor@master] Refine Codex usages and Vue components

https://gerrit.wikimedia.org/r/995355

Change 995357 had a related patch set uploaded (by Abaris; author: Abaris):

[mediawiki/extensions/Adiutor@master] Update @wikimedia/codex and @wikimedia/codex-icons to v1.3.1

https://gerrit.wikimedia.org/r/995357

Change 995357 merged by Abaris:

[mediawiki/extensions/Adiutor@master] Update @wikimedia/codex and @wikimedia/codex-icons to v1.3.1

https://gerrit.wikimedia.org/r/995357