Show resubmit channel to community library CTA after channel publish #5541
Conversation
AlexVelezLl
left a comment
AlexVelezLl
left a comment
There was a problem hiding this comment.
Thanks @taoerman! Looking good, just a little concerned about the KModal styles overrides, and noted a couple of nitpicks. Thanks!
| <template #actions> | ||
| <div class="resubmit-modal-actions"> | ||
| <KButton | ||
| class="resubmit-modal-dismiss-btn" | ||
| :style="dismissButtonStyles" | ||
| data-test="dismiss-button" | ||
| @click="handleDismiss" | ||
| > | ||
| {{ dismissButtonText }} | ||
| </KButton> | ||
| <KButton | ||
| class="resubmit-modal-resubmit-btn" | ||
| primary | ||
| data-test="resubmit-button" | ||
| @click="handleResubmit" | ||
| > | ||
| {{ resubmitButtonText }} | ||
| </KButton> | ||
| </div> | ||
| </template> |
There was a problem hiding this comment.
Is there any reason why we need these buttons to be in the #actions slot instead of using the KModal props? (It is not completely incorrect, it is more so because this slot is just for when we want specific styles handling on KModal actions)
There was a problem hiding this comment.
Thanks Alex, just using KModal props could make code simpler! Fixed!
| [data-test='resubmit-modal'], | ||
| [data-test='resubmit-modal'] > *, | ||
| [data-test='resubmit-modal'] [role='dialog'], | ||
| [data-test='resubmit-modal'] .KModal, | ||
| [data-test='resubmit-modal'] .modal { | ||
| width: 500px !important; | ||
| max-width: 500px !important; | ||
| max-height: 284px !important; | ||
| } | ||
| [data-test='resubmit-modal'] h1, | ||
| [data-test='resubmit-modal'] h2, | ||
| [data-test='resubmit-modal'] h3, | ||
| [data-test='resubmit-modal'] h4, | ||
| [data-test='resubmit-modal'] .modal-title, | ||
| [data-test='resubmit-modal'] [class*='title'] { | ||
| width: 452px; | ||
| height: 52px; | ||
| font-family: 'Noto Sans', sans-serif; | ||
| font-size: 20px; | ||
| font-weight: 600; | ||
| line-height: 130%; | ||
| letter-spacing: 0%; | ||
| vertical-align: middle; | ||
| } | ||
| .resubmit-modal-content { | ||
| padding: 8px 0; | ||
| margin-top: 35px; | ||
| } | ||
| .resubmit-modal-description, | ||
| .resubmit-modal-question { | ||
| width: 100%; | ||
| min-height: 40px; | ||
| padding: 0; | ||
| margin: 0; | ||
| font-family: 'Noto Sans', sans-serif; | ||
| font-size: 14px; | ||
| font-weight: 400; | ||
| line-height: 140%; | ||
| letter-spacing: 0%; | ||
| vertical-align: middle; | ||
| } | ||
| .resubmit-modal-actions { | ||
| display: flex; | ||
| gap: 10px; | ||
| justify-content: flex-end; | ||
| width: 452px; | ||
| min-height: 40px; | ||
| padding-top: 16px; | ||
| } | ||
| .resubmit-modal-dismiss-btn:hover { | ||
| background-color: var(--hover-bg-color) !important; | ||
| } | ||
| .resubmit-modal-resubmit-btn { | ||
| height: 40px; | ||
| } |
There was a problem hiding this comment.
Hmm, are all of these styles added to match exactly the Figma specs? I don't think we need almost any of them; we can rely on KModal's inner styles to have a consistent view across modals in the application.
There was a problem hiding this comment.
Yes, I added these styles to match Figma. But I found only using KModal's inner styles could also match Figma. Thanks Alex!
There was a problem hiding this comment.
Should the resubmit CL modal be checked and triggered by this component? If so, we can show a loader here, until that check finishes, and then we can emit a showResubmitCommunityLibraryModal or something like that.
There was a problem hiding this comment.
Added! But when I tried to test it manually. The process of publishing was so fast. I could not even see the loader. LOL!
| this.resubmitModalChannel = { | ||
| ...this.currentChannel, | ||
| version: latestSubmission.channel_version, | ||
| }; |
There was a problem hiding this comment.
We won't need to update the channel version in this way after this is merged!
| if (Array.isArray(response)) { | ||
| submissions = response; | ||
| } else if (response && response.results && Array.isArray(response.results)) { | ||
| submissions = response.results; | ||
| } |
There was a problem hiding this comment.
I don't think we need to check the response type. In this point of the application, it should always be the same.
| computed: { | ||
| title() { | ||
| return communityChannelsStrings.resubmitModalTitle$(); | ||
| }, | ||
| description() { | ||
| return communityChannelsStrings.resubmitModalBodyFirst$({ | ||
| channelName: this.channel.name, | ||
| version: this.channel.version, | ||
| }); | ||
| }, | ||
| question() { | ||
| return communityChannelsStrings.resubmitModalBodySecond$(); | ||
| }, | ||
| dismissButtonText() { | ||
| return communityChannelsStrings.dismissButton$(); | ||
| }, | ||
| resubmitButtonText() { | ||
| return communityChannelsStrings.resubmitButton$(); | ||
| }, | ||
| dismissButtonStyles() { | ||
| return { | ||
| color: this.$themeTokens.primary, | ||
| backgroundColor: this.$themePalette.grey.v_100, | ||
| '--hover-bg-color': this.$themePalette.grey.v_200, | ||
| }; | ||
| }, | ||
| }, | ||
| methods: { | ||
| handleDismiss() { | ||
| this.$emit('dismiss'); | ||
| this.$emit('input', false); | ||
| }, | ||
| handleResubmit() { | ||
| this.$emit('resubmit'); | ||
| this.$emit('input', false); | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Oh, and just one last thing! 😅 Could we migrate this to use the Vue Composition API instead? Thanks!
AlexVelezLl
left a comment
AlexVelezLl
left a comment
There was a problem hiding this comment.
Found a couple of things that can be changed to have a better consistency with other modals 👐 .
| value: { | ||
| type: Boolean, | ||
| default: false, | ||
| }, | ||
| channel: { | ||
| type: Object, | ||
| required: true, | ||
| validator(value) { | ||
| return value && typeof value.name === 'string' && typeof value.version === 'number'; | ||
| }, | ||
| }, | ||
| latestSubmissionVersion: { | ||
| type: Number, | ||
| default: null, | ||
| }, | ||
| }); | ||
| const emit = defineEmits(['dismiss', 'resubmit', 'input']); |
There was a problem hiding this comment.
Oooh, similar to what we did for the DeleteModal here, could we also use the @close event for the parent component to close the Modal instead of using dismiss and v-model? So we can keep this consistent pattern for rendering modals!
| latestSubmissionVersion: { | ||
| type: Number, | ||
| default: null, | ||
| }, | ||
| }); | ||
|
|
||
| const emit = defineEmits(['dismiss', 'resubmit', 'input']); | ||
|
|
||
| const title = computed(() => communityChannelsStrings.resubmitModalTitle$()); | ||
|
|
||
| const publishedVersion = computed(() => { | ||
| return props.latestSubmissionVersion != null | ||
| ? props.latestSubmissionVersion | ||
| : props.channel.version; | ||
| }); |
There was a problem hiding this comment.
latestSubmissionVersion should be required as well. If this prop is null, we should automatically close the side panel, since we need this information to be accurate when showing the modal; otherwise, it may confuse users.
| const mode = ref(PublishModes.LIVE); | ||
| const version_notes = ref(''); | ||
| const submitting = ref(false); | ||
| const checkingResubmit = ref(false); |
There was a problem hiding this comment.
I think we can just use the current submitting loading ref instead of creating a new one, because at the end, this process of checking the latest submission could be part of the "submission" process.
| <span | ||
| v-if="checkingResubmit" | ||
| class="loader-wrapper" | ||
| > | ||
| <KCircularLoader :size="16" /> | ||
| </span> | ||
| <span v-else>{{ submitText }}</span> |
There was a problem hiding this comment.
We don't usually change the text button to a circular loader while something is loading, so we can just skip it!
| } catch (error) { | ||
| store.dispatch('shared/handleAxiosError', error); | ||
| } finally { |
There was a problem hiding this comment.
Here, let's do nothing if something fails, let's make this call just optional, and if it fails, it shouldn't block the normal user workflow.
| if (submissions.length > 0) { | ||
| const latestSubmission = submissions[0]; | ||
| emit('showResubmitCommunityLibraryModal', { | ||
| channel: currentChannel.value ? { ...currentChannel.value } : null, |
There was a problem hiding this comment.
currentChannel.value should always be defined at this point, so lets just skip the null check
| const latestSubmission = submissions[0]; | ||
| emit('showResubmitCommunityLibraryModal', { | ||
| channel: currentChannel.value ? { ...currentChannel.value } : null, | ||
| latestSubmissionVersion: latestSubmission?.channel_version ?? null, |
There was a problem hiding this comment.
This latestSubmission ref should also always be defined at this point, the same with the channel_version, since this is a required field in the backend model, so that we can skip the null checks here too!
| } | ||
|
|
||
| function handleResubmit() { | ||
| emit('resubmit'); |
There was a problem hiding this comment.
Let's also emit the 'close' event when resubmitting, so that the parent component can handle both events separately, without needing to close the modal when the resubmit event is emitted.
| handleResubmit() { | ||
| this.showResubmitModal = false; | ||
| this.showSubmitToCommunityLibrarySidePanel = true; | ||
| }, | ||
| handleDismissResubmit() { | ||
| this.showResubmitModal = false; | ||
| this.resubmitModalChannel = null; | ||
| this.resubmitModalSubmissionVersion = null; | ||
| }, | ||
| handleShowResubmitModal({ channel, latestSubmissionVersion }) { | ||
| this.resubmitModalChannel = channel; | ||
| this.resubmitModalSubmissionVersion = latestSubmissionVersion || null; | ||
| this.showResubmitModal = true; | ||
| }, |
There was a problem hiding this comment.
Although less common, there is a more handy way to pass these multiple variables needed for showing the ResubmitChannelModal, and it is just storing the entire emitted object, and checking if this object is null to show the ResubmitChannelModal.
i.e., we could instead do something like this:
<PublishSidePanel
v-if="showPublishSidePanel"
@close="showPublishSidePanel = false"
@showResubmitCommunityLibraryModal="e => resubmitModalData = e" // we store the whole object without unwrapping it
/>And then in the ResubmitChannelModal rendering:
<ResubmitChannelModal
v-if="resubmitModalData" // We check if the `resubmitModalData` is defined
:channel="resubmitModalData.channel" // since at this point we know this ref is defined, we can access its properties
:latestSubmissionVersion="resubmitModalData.latestSubmissionVersion"
@resubmit="handleResubmit"
@close="resubmitModalData = null" // Set it to null instead of false since this variable is expected to be an object
/>With this, we prevent any potential regression because of data corruption (e.g., we set showResubmitModal but not resubmitModalChannel or resubmitModalSubmissionVersion), and save us some lines in the component.
| resubmitButton: { | ||
| message: 'RESUBMIT', | ||
| context: 'Button in the resubmit modal to open the submit to Community Library side panel', | ||
| }, | ||
| dismissButton: { | ||
| message: 'DISMISS', | ||
| context: 'Button in the resubmit modal to dismiss the modal', | ||
| }, |
There was a problem hiding this comment.
We usually use the Action suffix instead of Button, so for consistency, we can rename these strings to be resubmitAction and dismissAction.
And we can set the message to be just "Resubmit" and "Dismiss", and KButton will take care of using the upper case if needed.
AlexVelezLl
left a comment
AlexVelezLl
left a comment
There was a problem hiding this comment.
I swear this will be the last one! 😅
| <PublishSidePanel | ||
| v-if="showPublishSidePanel" | ||
| @close="showPublishSidePanel = false" | ||
| @published="handleChannelPublished" |
There was a problem hiding this comment.
Seems like handleChannelPublished isn't defined on the component. And I think it's because we don't need to do anything when the published event is emitted. So we can perhaps also remove the event emit within the PublishSidePanel component?
| if (resubmitData?.latestSubmissionVersion == null) { | ||
| this.showPublishSidePanel = false; | ||
| return; | ||
| } |
There was a problem hiding this comment.
In the PublishSidePanel component, we already emit the close event regardless of the showResubmitCommunityLibraryModal event, so the responsibility for closing the side panel is already handled; we can just set the resubmitData here.
There was a problem hiding this comment.
After rereading the code, I understand the logic!
- User clicks "Publish" in PublishSidePanel
- Channel gets published successfully
- PublishSidePanel emits showResubmitCommunityLibraryModal event with resubmit data
- handleShowResubmitModal receives the data and stores it: this.resubmitModalData = resubmitData
- PublishSidePanel emits close event
- TreeViewBase handles @close and sets showPublishSidePanel = false
- The ResubmitChannelModal appears
| :channel="currentChannel" | ||
| @close="showSubmitToCommunityLibrarySidePanel = false" | ||
| /> | ||
| <ResubmitChannelModal |
There was a problem hiding this comment.
I didn't catch this earlier, sorry. But the modal component to submit to the community library is called "SubmitToCommunityLibrarySidePanel", it is very specific in its name to express the intention of the component. I think we should do dat as well for the resubmit channel modal, and call the component ResubmitToCommunityLibraryModal and rename the resubmitModalData to be resubmitToCommunityLibraryModalData and the handleResubmit to handleResubmitToCommunityLibrary.
I know these names are becoming too verbose, but since we are adding these variables/methods/components within the scope of the TreeViewBase component, which is a general component, we should be specific about their context.
There was a problem hiding this comment.
(Just a note that in this refactor, we should also rename the ResubmitToCommunityLibraryModal file.
There was a problem hiding this comment.
Got it! I've renamed all required variables.
| const response = await CommunityLibrarySubmission.fetchCollection({ | ||
| channel: currentChannel.value.id, | ||
| max_results: 1, | ||
| }); |
There was a problem hiding this comment.
Just noting that if we don't wrap this in a try catch block, if this call fails, then the exception will be caught in this catch block, interrupting the normal user workflow, which may confuse. To prevent ourselves from interrupting the normal user workflow if this "optional" call fails, we should add this in a try ... catch block, but just do nothing in the catch block. For example.
import logging from 'shared/logging';
...
try {
const response = await CommunityLibrarySubmission.fetchCollection({
channel: currentChannel.value.id,
max_results: 1,
});
...
} catch (e) {
logging.error(e); // Just log the error, but do nothing to prevent blocking the user workflow.
}
AlexVelezLl
left a comment
AlexVelezLl
left a comment
There was a problem hiding this comment.
Code changes look good to me, and manual QA checks out. Thanks @taoerman!!
3 participants
Summary
Implemented ResubmitChannelModal to prompt users to resubmit a channel to the Community Library when publishing a channel that already has submissions. The modal appears immediately on publish click (not after completion, another issue has added a loader to make sure the side panel would load updated data from backend) and shows the channel name and the version already in the submission queue.
References
Fixed #5451
Reviewer guidance
test manually.