Page MenuHomePhabricator

Multiblocks: provide feedback and code review
Closed, ResolvedPublic3 Estimated Story Points

Details

Event Timeline

CCiufo-WMF triaged this task as Medium priority.Sep 16 2024, 5:05 PM
CCiufo-WMF set the point value for this task to 3.

Hey @MusikAnimal – could you provide a little more context and information about what sort of input you are looking for from us? I can do a quick pass to look at your usage of Vue.js and Codex to look for coding conventions, best practices, etc. but if you are looking for anything more specific let us know.

Special:Block lives in Core, but do any additional steps need to be taken to develop locally? Do you all tend to create multiple users with different "block" statuses on local instances while you are working on this feature?

Hey @MusikAnimal – could you provide a little more context and information about what sort of input you are looking for from us? I can do a quick pass to look at your usage of Vue.js and Codex to look for coding conventions, best practices, etc. but if you are looking for anything more specific let us know.

A quick review to ensure we're not doing anything dumb with Vue/Codex would be great!

But really this is about getting +2's. I guess we're having trouble getting reviews between (a) other engineers are busy with other priorities, (b) we are all new-ish to Vue, and those of who have used it (for the Community-Wishlist) are used to the Options API, and (c) the general anxiety that comes with merging code into Core. I asked for help getting code review and that's what led to this task being created. I don't think we're expecting DST to shepherd the whole project :) Maybe if you give just a +1 saying the Vue stuff looks OK, that will give others enough confidence to +2.

Special:Block lives in Core, but do any additional steps need to be taken to develop locally? Do you all tend to create multiple users with different "block" statuses on local instances while you are working on this feature?

You need $wgUseCodexSpecialBlock = true in your LocalSettings.php, then simply browse to Special:Block. I don't think it's necessary to have different users to test. Maybe you'd want one that is blocked to test prefilling the form, but that isn't implemented yet (T368583). If it wasn't clear, we're currently only building the Vue facelift of the existing Special:Block, i.e. no Multiblocks functionality.


While I've got you here, though… I do have a specific question. In r1074256 I'm working on validations. I will look into a store for the state at some point, but that aside, I wanted to leverage native clientside form validation to avoid adding new i18n messages and other reinvention of the wheel. Essentially I wanted to "extend" the TextInput component so that it reacts to isValid events. I thought a composable might have been the way to do it (with usage like const ValidatingTextInput = useSelfValidation( CdxTextInput )), but I could not figure that out. I ended up doing some rather low-level stuff in a new dedicated component called ValidatingTextInput. It works, but I'm still unsure if there's a more preferred way to do this.

Hey @MusikAnimal – could you provide a little more context and information about what sort of input you are looking for from us? I can do a quick pass to look at your usage of Vue.js and Codex to look for coding conventions, best practices, etc. but if you are looking for anything more specific let us know.

A quick review to ensure we're not doing anything dumb with Vue/Codex would be great!

Great, I just posted some reviews on the first patch in the chain and can spend more time with these this week. Thanks for providing some context here.

While I've got you here, though… I do have a specific question. In r1074256 I'm working on validations. I will look into a store for the state at some point, but that aside, I wanted to leverage native clientside form validation to avoid adding new i18n messages and other reinvention of the wheel. Essentially I wanted to "extend" the TextInput component so that it reacts to isValid events. I thought a composable might have been the way to do it (with usage like const ValidatingTextInput = useSelfValidation( CdxTextInput )), but I could not figure that out. I ended up doing some rather low-level stuff in a new dedicated component called ValidatingTextInput. It works, but I'm still unsure if there's a more preferred way to do this.

You may be interested in T373872 (which one of the WMDE devs just filed a few weeks ago). @ItamarWMDE wrote a couple of patches to expose the native HTML form APIs in the TextInput component specifically (T374402) that we cam probably merge in our next sprint (so not this week but the week or two after that). Take a look at these tasks/patches and let me know if that covers your use case here.

Ideally we can do things in a way that no one has to re-invent any wheels here.

Codex components do support a lot of native validation behavior, but if you can't use it for whatever reason then having a way to access the native browser API seems handy.

You may be interested in T373872 (which one of the WMDE devs just filed a few weeks ago). @ItamarWMDE wrote a couple of patches to expose the native HTML form APIs in the TextInput component specifically (T374402) that we cam probably merge in our next sprint (so not this week but the week or two after that). Take a look at these tasks/patches and let me know if that covers your use case here.

Awesome! I was going to ask if this ValidatingTextInput was worthy of upstreaming. I'm happy to see the same concepts are being adopted.

The patches for T373872 seem to mostly do what I want, except they don't set the status value, so I would still need to "extend" TextInput as I am in r1074256 to keep things DRY. That's okay, but I wonder if we could take this standardization a step further. I've commented at T373872#10172370.

Change #1075345 had a related patch set uploaded (by Eric Gardner; author: Eric Gardner):

[mediawiki/core@master] [DNM] SpecialBlock: improved data flow for UserLookup.vue

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

Change #1075345 abandoned by Eric Gardner:

[mediawiki/core@master] SpecialBlock: improved data flow for UserLookup.vue

Reason:

It seems like this patch has served its purpose; development work continues at https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1075084

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