-
Notifications
You must be signed in to change notification settings - Fork 215
Update channel publishing UX to support publishing channel draft versions #5274
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
base: community-channels
Are you sure you want to change the base?
Update channel publishing UX to support publishing channel draft versions #5274
Conversation
…ions fix some typo fix a bug modal fix some bugs fix frame bug
6cf5b0c
to
8352770
Compare
update tests for publish draft using main tree Enhanced publish functionality to support three publish types fixed fix frontend tests
…ions fix some typo fix a bug modal fix some bugs fix frame bug
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 @taoerman! Apologies, I forgot to add some instructions in the issue description regarding some of the patterns we want to use in new components. If you have any question, please let us know!
@@ -0,0 +1,267 @@ | |||
<template> | |||
<div> | |||
<SidePanelModal |
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.
Apologies I didn't link how we use this component in other parts. This was a copy of the SidePanelModal component we have in Kolibri. Here you can see some examples of how we use it there. So there are couple of differences here, for example, we dont set the aria label to the modal, because its label is actually defined in the <hX>
tag in the header. Also The side panel internally already handles the keyup.esc
event
@closePanel="onClose" | ||
> | ||
<template #header> | ||
<h2 style="margin: 0">{{ getPanelTitle() }}</h2> |
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.
We treat a side panel as another page, so we usually use an h1
tag for its title
|
||
<template #default> | ||
<div class="form-section"> | ||
<VRadioGroup v-model="mode"> |
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.
Apologies for not flagging this in the issue (I thought I had done it 😓). Currently, most of the Studio frontend codebase is built with Vuetify. However, for all these new features, we want to prioritize using our Kolibri Design System (KDS) as much as possible (unless relevant shared components already exist). So for example, for this RadioGroup, we can use our KRadioButtonGroup, and for radio buttons we can use our KRadioButton, and so on.
|
||
<div class="form-section" style="width: 100%; max-width: 100%;"> | ||
<div class="textarea-container"> | ||
<label class="label">{{ $tr('versionNotesLabel') }}</label> |
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.
Here, we can also replicate the way the previous publish modal renders its description textarea using the KTextbox component
<textarea | ||
v-model="version_notes" | ||
:rows="4" | ||
:maxlength="30" |
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 know the designs shows a maxlength of 30, but it is too short. We can keep it in 250 instead.
submitting: false, | ||
}; | ||
}, | ||
computed: { |
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 is something I also forgot to describe in the issue, but we also encourage the use of the Vue Composition API for new components, though using the Options API is acceptable if you find it more comfortable :). Here you can find an example of how we use it in Kolibri.
}, | ||
}, | ||
$trs: { | ||
publishToLibrary: 'Publish to library', |
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 all these new strings, it will be better if we store them in the communityChannelsStrings file. Here you can find an example of how we usually build these strings files. And then, if we use the composition api, we can import this file, and destructure the needed string appending a "$" (e.g. here), and this is a function we can invoke to get the string (e.g. here).
Hi @AlexVelezLl , I have fixed the code according to you comments. At the same time, I added language selector. Thanks! |
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 @taoerman! The draft publishing is working good! But there are some tweaks we can do in the code to follow some of our conventions in LE. I have noted most of them, as always, if you have any questions please let us know.
Also, I see that the lint check is not working properly. Could you please run pnpm lint-frontend:format
to correct the code formatting? Thanks!
<!-- Live mode content nested under the radio button --> | ||
<div v-if="mode === 'live'" class="live-mode-content" style="margin-left: 24px; margin-top: 16px;"> | ||
<div class="info-section"> | ||
<VIconWrapper class="info-icon">info</VIconWrapper> |
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.
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! I used KIcon instead already.
|
||
<div v-if="incompleteResourcesCount > 0" class="form-section warning-section"> | ||
<div class="warning-content"> | ||
<VIconWrapper class="warning-icon">warning</VIconWrapper> |
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.
Idem
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.
same to here.
|
||
<template #bottomNavigation> | ||
<div class="footer"> | ||
<VBtn flat @click="onClose">{{ cancel$() }}</VBtn> |
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.
Here we can use our KButton component instead :)
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 have changed all VBtn in PublishSidePanel to KButton.
<div v-if="mode === 'live'" class="live-mode-content" style="margin-left: 24px; margin-top: 16px;"> | ||
<div class="info-section"> | ||
<VIconWrapper class="info-icon">info</VIconWrapper> | ||
<span>{{ publishingInfo$({ version: currentChannel.version, time: formattedPublishTime }) }}</span> |
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.
(Apologies for deviating from the figma specs, but can we show the next version instead? e.g. "You are publishing: Version 5".
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.
That make sense, thanks!
import VIconWrapper from 'shared/views/VIconWrapper'; | ||
import KRadioButtonGroup from 'kolibri-design-system/lib/KRadioButtonGroup'; | ||
import KRadioButton from 'kolibri-design-system/lib/KRadioButton'; | ||
import KTextbox from 'kolibri-design-system/lib/KTextbox'; |
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.
All our KDS components are globally available, and we don't need to declare them in each component, as they are globally registered when we install KDS! https://github.com/misrob/kolibri-design-system/blob/9003ff6a56d99451be5f607a6f3b14baa0dbb4b9/lib/KThemePlugin.js#L126.
So we can skip these imports here!
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.
Yes, i have deleted them.
channelId: { type: String, required: true }, | ||
}, | ||
emits: ['close', 'submitted'], | ||
setup(props, { emit }) { |
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.
great! 🎉
}; | ||
|
||
const getPanelTitle = () => { | ||
return mode.value === 'draft' ? publishToLibrary$() : publishChannel$(); |
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 think we can keep the publishChannel$
title for both modes.
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.
got it! that make sense!
.radio-description { | ||
margin-left: 24px; | ||
margin-bottom: 16px; | ||
color: rgba(0, 0, 0, 0.6); |
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.
We don't usually use burned colors in the styles. Instead we have the $themePalette
and $themeTokens
global variables that we can use to manage dynamic branding colors https://design-system.learningequality.org/colors/#usage. Here you can see the tokens, and here the whole palette. Here you can see some examples of how we use them. We usually just add them as inline styles in the template, but if you want to define any variable inside the setup method, we usually do this instead (importing the themeTokens function from KDS).
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 your link!
const change = new PublishedChange({ | ||
key: id, | ||
version_notes, | ||
language: channel.language || currentLanguage, |
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.
The language in the publish method is just used to set the language of the email that will be sent to the user once the publish has finished. So I don't think the desired behavior here would be to prioritize the channel.language for this.
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.
got it!
}); | ||
}, | ||
|
||
_monitorDraftCompletion(channelId) { | ||
const checkInterval = setInterval(async () => { |
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.
(Non blocking) Seems like there is a liveQuery
we can use instead of this setInverval
return this.table |
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.
got it. I've used liveQuery instead of setInterval.
Thank you so much, @AlexVelezLl ! I have fixed the code according to your comments. Additionally, pnpm run test-jest and pnpm lint-frontend:format both passed! I have learned a great deal about frontend development in this issue, especially Vue. Thanks again! |
@@ -1209,45 +1209,86 @@ export const Channel = new CreateModelResource({ | |||
}); | |||
}, | |||
|
|||
publish(id, version_notes) { | |||
publish(id, version_notes, language) { |
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.
if I do not update publish function. After publishing, the language in change table would be "en" default.
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.
Not necessarily, it will be set to the currentLanguage
which is the language of the channel editor's UI. We should send the email in the language the user is currently seeing. If the user has set the system language to spanish, the success email should be sent in spanish, even if the language of the channel they were editing is english.
Also, there is a distinction here, one thing is the content language, i.e. the language the resources, folders, etc has. And a very different thing is the system language, which is the language the UI is showing to the user. We have support for ~285 content languages, i.e. the user can create resources for any of these 285 languages. But we have support just for 5 system languages, i.e. the user can see the studio interface just in 5 languages. So, if we are publishing a channel which language is in "Acholi", the email will try to be translated to this language, but since we dont have system language support for "Acholi", it will very likely fail to send the email, or it will send the email in a weird format.
So thats why here we should only rely on the currentLanguage
, no matter what language the user selected on the publish modal, or what language is currently set in the channel
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.
got it!
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 @taoerman! We're getting there!! After this I think the implementation will be pretty much the same as the design specs! 😃
<span>{{ | ||
publishingInfo$({ | ||
version: currentChannel.version + 1, | ||
time: formattedPublishTime, |
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.
(Apologies for deviating from the designs 😓) Here we should not show the current time for the publish version, it should only say "(i) You're publishing: Version 1" Without showing the current datetime
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've removed current time.
:invalidText=" | ||
version_notes.length === 0 ? 'Version notes are required' : maxLengthError$() | ||
" | ||
:showInvalidText="version_notes.length === 0 || version_notes.length > 250" |
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.
There is a little, small UX problem with setting this validation directly here, and is that this validation is shown inmediately after the user open the side panel, but the user hasn't made any error yet, so we should show this just when the user has already interacted (i.e. blurred) the input. The previous publish modal implemented a "lazy validation", by creating a ref variable to set if the invalid text should be showed or not, and then they set this value on the submit handler, we can do the same, but in the @blur event that is emitted from the KTextbox.
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've added lazy validation into version notes.
<div | ||
class="warning-content" | ||
:style="{ | ||
backgroundColor: $themeTokens.warning, |
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 a couple of design changes we need to fully align the implementation with the specs:
- The background color of this warning should be $themePalette.yellow.v_100. Without border.
- The warning Icon should have the color $themePalette.yellow.v_500.
- The border radius should be 4px.
- The
incompleteResourcesDescription
should be separated in two strings and added in two different paragraphs, to show two lines. We can name itincompleteResourcesDescription1
andincompleteResourcesDescription2
, orincompleteResourcesDescription
andincompleteResourcesCallToAction
, the way you prefer!
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've fixed the design.
</template> | ||
|
||
<template #bottomNavigation> | ||
<div class="footer"> |
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.
<template #bottomNavigation> | ||
<div class="footer"> | ||
<KButton | ||
flat |
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.
Here the way we have to set a flat button is using the appearance prop set to flat-button
. But we don't need it here.
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've added width: 100% and used appearance prop.
message: 'Publish channel', | ||
}, | ||
publishLive: { | ||
message: 'PUBLISH', |
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.
The KButton component already takes care of setting the letter case for the label, so here we can just set the message to "Publish". The same with "Save draft".
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.
got it!
message: "You're publishing: Version {version} ({time})", | ||
}, | ||
incompleteResourcesWarning: { | ||
message: '{count} incomplete resources.', |
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.
To support the distinction between plural vs singular messsages, we can replace this message to be:
message: '{count} incomplete resources.', | |
message: '{count, number} {count, plural, one {incomplete resource} other {incomplete resources}}.', |
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, I've fixed according to comment.
"Incomplete resources will not be published and made available for download in Kolibri. Click 'Publish' to confirm that you would like to publish anyway.", | ||
}, | ||
maxLengthError: { | ||
message: 'Maximum 250 characters allowed', |
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 saw that we don't have any max character limits error. It seems that our KTextbox will never have a value with more than the maxLength characters long
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.
got it!
> | ||
<KIcon | ||
icon="warning" | ||
:style="{ color: $themeTokens.warningText }" |
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.
Here we should use the prop color
instead of setting the color on the styles.
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've used prop color instead of style.
maxLengthError: { | ||
message: 'Maximum 250 characters allowed', | ||
}, | ||
cancel: { |
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.
Could we name this message cancelAction
instead? Thanks!
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've changed cancel to cancelAction.
Hi @AlexVelezLl , thanks for your review and patience. I have fixed the code according to your comments. |
Summary
Enhanced publish functionality to support "Live" and "Draft" publish types. Draft publishes save content without marking it as published or incrementing channel version. Updated PublishSidePanel.vue with mode selection, dynamic titles, and improved status display in ProgressModal.vue showing "Saving draft..." for drafts and "Published X minutes ago" for live publishes.
Manual verification: Tested draft publish functionality, confirmed PublishedNextChange entry creation, verified channel version remains unchanged, and main tree stays unpublished. PUBLISH button correctly remains enabled after draft save for subsequent live publishing.
References
#5209
Reviewer guidance
Test locally