Settings: new user row replaced by a modal

[Simounet]

Hi there,
I know that I have to commit the compiled files but I need some help here. make watch-js changes files without me touching anything.
Another issue is that on modal opening, the HTML validation displays error on password and mail inputs. Did I miss something here because it wasn't an issue before.

Here is a screenshot of the result:

Linked to #23397.

Have a good night and stay safe.

[kesselb]

/compile amend /

[kesselb]

make watch-js changes files without me touching anything.

I have the same issue from time to time. Usually fixed by rm -rf node_modules. Maybe not bad to use node 12.x. Super cool enhancement and much better then before 👍

[Simounet]

I tried to clean my env but it's not working. I'm running Node v12.14.1.

[ghost]

@jancborchardt What do you think about this?

[Simounet]

I put the display name first and the popin is larger.

The last issue here is about the HTML validation when the popin appears.

[kesselb]

cc @skjnldsv any idea why the form validation triggers on modal open?

[skjnldsv]

cc @skjnldsv any idea why the form validation triggers on modal open?

Probably a chaining events. Make sure you use an event stop on the button

[kesselb]

Or it's probably the default behaviour 🤔

https://developer.mozilla.org/en-US/docs/Learn/Forms/Form_validation
https://mdn.github.io/learning-area/html/forms/form-validation/full-example.html

Each element with required get the :invalid state.

[skjnldsv]

Ah, I misunderstod "form validation". I understood that as "submit". :)
Yes, it's normal if you use field types

[Simounet]

I get what was bothering me. Previous style override the red border so I put that here. It's not perfect but it's better than having red border without touching anything.

[ghost]

I put the display name first and the popin is larger.

The last issue here is about the HTML validation when the popin appears.

Looks good!! Nice work

[skjnldsv]

Seems like an odd change, why was it needed?

[Simounet]

Because of the default border of 1px on each side. Without changing the box sizing, I have 2 extra pixels.

[skjnldsv]

A few comments. Please abuse comments on your css code, especially v-deeps. Otherwise we don't know why you had to do it and maintaining is complicated :)

Same for other functions, it's a good habit to put more comments. It will get stripped for the build anyway, file size doesn't matter here.

[Simounet]

Ahah, some people love comments where some others don't. I will remember that they are appreciated here. :)

[ghost]

@Simounet is there still an issue with the code ?

[skjnldsv]

I think the acceptance test fail is valid.
@Simounet can you rebase?

[julien-nc]

Code looks sane, the modal is much nicer!

[Simounet]

I rebased and push. Just waiting for the CI.

[danxuliu]

Sorry a lot for the delay.

#25991 updates the browser used in the acceptance tests to a recent version. With that recent version the dialog works as expected :-)

However, turns out that the header test was wrong! It was trying to add the user "user1", but that user is already added when installing and configuring the server, so it is already added in all tests. Thus, even if the dialog worked properly, the test would also fail because adding the user would fail, the dialog would not be closed, and the following steps would fail. The header test is fixed by #25990

Due to all that this pull request should be rebased on master once #25990 and #25991 are both merged and everything should then work (I have also added a fixup commit to this pull request to do a minor adjustment).

[Simounet]

I can't wait to see this PR merged! Thanks for the reply. I don't get why you removed my test to check if the modal is not here. Is it somewhere else on your PRs?

[kesselb]

Hi @Simounet,

mind to rebase your branch another time?

[danxuliu]

#25991 has been finally merged, so I took the liberty to rebase this pull request to latest master :-)

I don't get why you removed my test to check if the modal is not here. Is it somewhere else on your PRs?

Sorry, I missed the question. I removed the check because I think that it is not strictly needed in that scenario. When possible the acceptance tests should not go into specific details like whether a modal is used or not, and in this case it should be enough to just verify that the user was indeed added to the list to continue.

Of course it could happen that the modal was not properly closed, so a check for that could still be added in the scenarios related to user creation, or inside the step I create user :user with password :password (as that is a high level step that abstracts a "complex" action, and the modal being closed would be part of that action). But for me it still feels like something not being tested at the right scope, so I did not add it :-)

[ghost]

this is finally happening 🙏🏻😁 super excited

[skjnldsv]

All tests passed! Well done!! 🎉 🚀

[Simounet]

Glad to see this finally merged! Thank you everyone for your support in my first real PR on this amazing piece of software.
Hope you'll enjoy this "feature" @andyxheli ! :D

[ghost]

Glad to see this finally merged! Thank you everyone for your support in my first real PR on this amazing piece of software.

Hope you'll enjoy this "feature" @andyxheli ! :D

Thank you so much brother for making this happen. Definitely will 😁 have a nice day.