-
Notifications
You must be signed in to change notification settings - Fork 252
[Remove Vuetify from Studio] Send e-mail dialog #5487
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
[Remove Vuetify from Studio] Send e-mail dialog #5487
Conversation
LianaHarris360
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for updating the EmailUsersDialog component. This is a good start so far. There’s still a couple of Vuetify components that can be removed. The VBtn and VCardText can be replaced with KButton and a div. There’s also a few user-facing strings that still need to be translated. The KModal title, the From and To labels within the form, “Subject line”, “Email body”, and the phrase “Add placeholder to message”, and all of the placeholder labels.
contentcuration/contentcuration/frontend/administration/pages/Users/EmailUsersDialog.vue
Show resolved
Hide resolved
contentcuration/contentcuration/frontend/administration/pages/Users/EmailUsersDialog.vue
Outdated
Show resolved
Hide resolved
contentcuration/contentcuration/frontend/administration/pages/Users/EmailUsersDialog.vue
Show resolved
Hide resolved
contentcuration/contentcuration/frontend/administration/pages/Users/EmailUsersDialog.vue
Outdated
Show resolved
Hide resolved
contentcuration/contentcuration/frontend/administration/pages/Users/EmailUsersDialog.vue
Outdated
Show resolved
Hide resolved
...ation/contentcuration/frontend/administration/pages/Users/__tests__/emailUsersDialog.spec.js
Outdated
Show resolved
Hide resolved
contentcuration/contentcuration/frontend/shared/views/StudioChip.vue
Outdated
Show resolved
Hide resolved
contentcuration/contentcuration/frontend/shared/views/StudioChip.vue
Outdated
Show resolved
Hide resolved
contentcuration/contentcuration/frontend/shared/views/StudioChip.vue
Outdated
Show resolved
Hide resolved
I have a doubt related #5426 (comment) this.Here i was told that wrapped strings are not to be used in administraion.So please do tell what i need to do here in this context |
MisRob
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Chiming in with some additional notes.
Overall congratulations on your first more complex issue. Overall nice work. All main features seem to be preserved Thanks for examining the current experience carefully.
contentcuration/contentcuration/frontend/administration/pages/Users/EmailUsersDialog.vue
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this file, I would recommend to take the same approach that I suggested in your other PR. For the same reasons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the test file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this is nicely written, clear and helpful test suite.
contentcuration/contentcuration/frontend/shared/views/__tests__/StudioChip.spec.js
Outdated
Show resolved
Hide resolved
contentcuration/contentcuration/frontend/shared/views/__tests__/StudioChip.spec.js
Outdated
Show resolved
Hide resolved
contentcuration/contentcuration/frontend/shared/views/StudioChip.vue
Outdated
Show resolved
Hide resolved
contentcuration/contentcuration/frontend/shared/views/StudioChip.vue
Outdated
Show resolved
Hide resolved
contentcuration/contentcuration/frontend/administration/pages/Users/EmailUsersDialog.vue
Outdated
Show resolved
Hide resolved
contentcuration/contentcuration/frontend/administration/pages/Users/EmailUsersDialog.vue
Outdated
Show resolved
Hide resolved
contentcuration/contentcuration/frontend/administration/pages/Users/EmailUsersDialog.vue
Outdated
Show resolved
Hide resolved
contentcuration/contentcuration/frontend/administration/pages/Users/EmailUsersDialog.vue
Outdated
Show resolved
Hide resolved
f701169 to
86e2c7f
Compare
|
Hey @MisRob, just a quick note — when triggering the email dialog from the UserActionDropdown for a specific user, their name doesn’t show up as a chip in the “To” field.
which actually should be
|
Oversight, I would say. |
|
Should i update it? |
|
@Prashant-thakur77 That'd be nice, thank you. |
|
Hi @Prashant-thakur77, just checking in on what's status or if you need re-review from @LianaHarris360. From brief skim, it seems you've resolved most of the feedback. I only don't see changes related to this comment - is it still work in progress, or some clarification is needed? |
YES @MisRob do i need to make whole area clickable chips for this as in StudioChip component because i have already implemented it using button as you mentioned and this has been updated(the button component is in EmailUsersdialog not in studiochip) and the chips with x button have been implemented with help of kicon button in StudioChip component.And those chips without clear button such as used in (in front of front text )or when just one recepient is present are also in Studiochip. The chips are working as expected.And you also mentioned to remove the extra computed effects such as(darken on clicking) and will further notify during review :) |
apart from it everything else if fine. |
|
Ah I see @Prashant-thakur77. It's good consideration. I think that having |
LianaHarris360
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for updating your PR and resolving the feedback, it's almost ready to be merged! There were some visual elements that are slightly different from the initial modal that I commented on.
contentcuration/contentcuration/frontend/administration/pages/Users/EmailUsersDialog.vue
Outdated
Show resolved
Hide resolved
contentcuration/contentcuration/frontend/administration/pages/Users/EmailUsersDialog.vue
Outdated
Show resolved
Hide resolved
contentcuration/contentcuration/frontend/shared/views/StudioChip.vue
Outdated
Show resolved
Hide resolved
contentcuration/contentcuration/frontend/administration/pages/Users/EmailUsersDialog.vue
Show resolved
Hide resolved
a8aad03 to
1158ecb
Compare
LianaHarris360
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This requested changes have been made and everything looks good to me, thank you @Prashant-thakur77!
MisRob
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Prashant-thakur77 and @LianaHarris360, overall it's in a good shape. I'm leaving few cleanup notes for code. As for user experience, I'd like to ask to fine-tune below, then we should be good for merge.
| wrapper.vm.show = false; | ||
| expect(wrapper.emitted('input')[0][0]).toBe(false); | ||
| }); | ||
| await user.click(screen.getByText('Send email')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getByRole would be better for buttons. It's more specific and aligned with VTL's query priority guidance. I think there are more places like this in the test suite. I will point to some of them but likely not all - please revisit the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated..
| expect(sendEmail).not.toHaveBeenCalled(); | ||
| const messageInput = screen.getByLabelText(/email body/i); | ||
| await user.type(messageInput, 'Hello '); | ||
| await user.click(screen.getByText('First name')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getByRole
Related to the other comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated it and other places..
| await user.type(messageInput, 'Hello '); | ||
| await user.click(screen.getByText('First name')); | ||
|
|
||
| expect(messageInput.value).toContain('Hello'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this test case you attempt to test for 'adds placeholder text to message when clicked', and you correctly clicked First name button, but this expect doesn't test that the placeholder is there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated it
| await user.click(screen.getByText('Cancel')); | ||
| await user.click(screen.getByRole('button', { name: /discard draft/i })); | ||
|
|
||
| expect(emitted().input?.[0]).toEqual([false]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to understand - what's this line testing for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MisRob Thanks for checking! The reason I tested the emitted event here is because, in this component, the actual responsibility is to emit input: false when the user confirms discarding the draft. The parent normally handles closing the dialog,
So this line:
expect(emitted().input?.[0]).toEqual([false]);
just confirms that the component is correctly requesting to close itself following v-model pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah okay, that makes total sense. Thanks for explaining. I didn't connect the dots.
It's probably clear from the test itself, even though updating the test description to emits input event with false value when discard confirmed may help a bit too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Appreciate the suggestion — it definitely improves readability,
And updated it :)
| <KModal | ||
| v-if="show" | ||
| :size="600" | ||
| title="Send Email" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| title="Send Email" | |
| title="Send email" |
Let's preserve the current case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed it
| label="Subject line" | ||
| :required="true" | ||
| :invalid="errors.subject" | ||
| :showInvalidText="showInvalidText && errors.subject" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shows the validation text even if the user has not focused or changed the input
So it's not clear to me why there's the need to set showInvalidText dynamically. Can't it simply be set to true at all times if that's what you're trying to achieve?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right...I got mixed up there initially. Setting showInvalidText dynamically wasn’t necessary, and keeping it always true makes more sense in this context. I've updated it now. Thanks a lot for pointing it out!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this is nicely written, clear and helpful test suite.
| <template> | ||
|
|
||
| <div | ||
| ref="chip" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this ref is used from anywhere? If so, can be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated it
| border-radius: 12px; | ||
| transition: all 0.2s ease; | ||
| &__content { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see here on BEM - not only for this PR, but also other ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated it
|
|
||
| test('renders close button when close prop is true', () => { | ||
| renderComponent({ close: true }); | ||
| expect(screen.getByLabelText('Remove Test Chip')).toBeInTheDocument(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're trying to test for button presence, I'd recommend to query with getByRole. Same for other tests in this file that refer to buttons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated it
|
@Prashant-thakur77 when implementing the chip 'Remove' button, see if our themed blue focus ring gets applied automatically to the button on keyboard navigation. If not, you will need to do something similar to |
@MisRob I tracked down the reason the bold text appears inside EmailUsersDialog. The dialog is currently rendered inside an h1 in UserTable.vue, so it ends up inheriting the font-weight-bold styling from the header. The Problem studio/contentcuration/contentcuration/frontend/administration/pages/Users/UserTable.vue Lines 4 to 21 in d9f08c3
There are a couple of ways we could address this—either by restructuring the component placement so the dialog isn’t nested inside an h1 or Update KModal in KDS I tried the first one and it works: |
@MisRob I’ve updated the remove button by using two KIcon elements one grey and one black,and I just toggle their opacity on hover. This gives the clean grey, black hover effect previously done, without the oversized hover circle from KIconButton. Let me know if you'd like any tweaks!
Below is how it was done before |
|
As for the modal within Regarding the close button - yes to your strategy. I've just recalled I did the same in another place actually, |
1158ecb to
fbdb955
Compare
MisRob
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice @Prashant-thakur77. Pity I didn't recall much sooner that we have HoverIcon at hand :) Thanks for checking on other places where it was used - I too confirm no regressions. All latest changes are meaningful.
One last detail I noticed - there's blue focus ring missing around placeholder buttons. After you've committed the related code suggestions, we can merge.
| :key="`placeholder-${placeholder.label}`" | ||
| class="placeholder-button" | ||
| :style="placeholderButtonStyles" | ||
| :class=" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:class="
$computedClass({
':hover': { backgroundColor: $themePalette.grey.v_300 },
':focus': { ...$coreOutline, outlineOffset: 0 }
})
"
This will make blue focus ring appear around the placeholder buttons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated it
|
@MisRob done the desired changes |
MisRob
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you both.
@Prashant-thakur77 extra thanks for patience while we were iterating on whether to use or not use some KDS components, and also fixing some pre-existing bugs along the way. Finally, StudioChip will be useful in many other places.






Fixes #5425
Summary
This PR completes the removal of Vuetify from the Send Email dialog as part of the larger Vuetify migration effort (#5060). The dialog now uses Kolibri Design System components exclusively while maintaining all existing functionality.
Changes Made:
✅ Replaced VDialog and ConfirmationDialog with KModal
✅ Replaced VFlex and VLayout with custom CSS flex styles
✅ Replaced VForm with native form element
✅ Replaced VTextField and VTextarea with KTextbox
✅ Replaced VTooltip with KTooltip
✅ Implemented form validation using generateFormMixin
✅ Created new StudioChip component to replace VChip



References
Sub-issue of #5060.
Reviewer guidance
Login as [email protected] with password a
Go to Administration > Users
Select few users in the table
Click Email
Visual Changes:
Minor styling differences due to KDS vs Vuetify
Consistent with Kolibri Design System patterns
Maintains all existing functionality