Info messages about previous submissions in create submission side panel #5513
Conversation
There was a problem hiding this comment.
Thanks @taoerman!! This is looking :chefs-kiss:. I've found a couple of minor issues that can be improved, but overall, this looks great!!
| .box { | ||
| padding: 8px; | ||
| color: v-bind('boxTextColor'); | ||
| padding: 12px 16px; |
There was a problem hiding this comment.
Here, the box should have a vertical and horizontal padding of 10px to match the specs styles.
| class="box-content" | ||
| > | ||
| <div class="box-icon"> | ||
| <KIcon :icon="icon" /> |
There was a problem hiding this comment.
Could we set the font-size of this icon to 18px? Right now, it feels a bit small compared to the spec.
| v-else-if="latestSubmissionIsFinished" | ||
| class="info-section" | ||
| > | ||
| <div class="info-text"> |
There was a problem hiding this comment.
Could we set the color of this node to gray.v_700 to match the specs? Thanks!
| <div | ||
| class="more-details-link" | ||
| data-test="more-details-button" | ||
| @click="showingMoreDetails = true" | ||
| > | ||
| {{ moreDetailsButton$() }} | ||
| </div> |
There was a problem hiding this comment.
Could we use the KButton component with the appearance set to basic-link? That'd work fine, although the specs do not look as a link, but it's okay.
| <div class="info-text"> | ||
| <template v-if="latestSubmissionStatus === null"> | ||
| <template v-if="showingMoreDetails"> | ||
| <div>{{ infoBoxPrimaryText }} {{ moreDetails$() }}</div> |
There was a problem hiding this comment.
Could we show the moreDetails$ in a different paragraph? Rather than having it next to the primary text, separated by a space.
| <div class="channel-title">{{ channel.name }}</div> | ||
| <div> | ||
| <div class="field-annotation">{{ languagesDetected$() }}</div> | ||
| <div class="channel-title">{{ channel.name }} v{{ channel.version }}</div> |
There was a problem hiding this comment.
Having the title string composed by
{{ channel.name }} v{{ channel.version }} is not internationalizable, because if in any other languages a different character is used to separate text, or a different character is used to display versions, then this structure won't apply.
I think we can perhaps create a new "channelVersion$" string that receives the channel name and its version, so it can be translated properly.
There was a problem hiding this comment.
Thanks Alex, I created a placeholders {name} and {version} in communityChannelsStrings.js.
| ); | ||
| const canBeEdited = computed(() => { | ||
| if (!isPublished.value || isPublic.value) return false; | ||
| return !isCurrentVersionAlreadySubmitted.value || needsReplacementConfirmation.value; |
There was a problem hiding this comment.
Here, I think we can allow editing the form, but prevent the user from submitting it, so they don't have to check this first before editing anything else in the modal.
| replacementConfirmed.value = false; | ||
| description.value = ''; |
There was a problem hiding this comment.
I don't think we need to clear these values here. Because by this point, the modal was already unmounted, it doesn't make any difference.
| color: #4368f5; | ||
| cursor: pointer; | ||
| } | ||
| .more-details-link:hover { | ||
| color: #4368f5; |
There was a problem hiding this comment.
I think these styles will be removed because we may use KButton instead, but just as a general note: Something very weird should happen for us to use burned colors, in the vast majority of cases, we will always use the themeTokens or themePalette colors.
| constraints = [ | ||
| models.UniqueConstraint( | ||
| fields=["channel", "channel_version"], | ||
| name="unique_channel_with_channel_version", | ||
| ), | ||
| ] |
There was a problem hiding this comment.
Whoops, it seems I didn't explain this well 😅. Apologies. This constraint still applies; we should not allow creating multiple submissions of the same channel version. If the user wants to create a new submission for the same channel, they must publish a new version first.
There was a problem hiding this comment.
Ah, I see. There is little issue I found if we still use props.channel.version to get channel version, it would get the previous channel version. So I used a computed property of currentChannel to instead of. It works much better than props.channel.version.
There was a problem hiding this comment.
Hi Alex, I have fixed the logic of checkbox. Only show checkbox if there's a pending submission AND the version has changed.
2 participants
Summary
References
Fixes #5450
Reviewer guidance
run new unit tests;
test the updated UI manually.