Submit to Community Library panel

[Jakoma02]

Summary

This PR implements a side panel for submitting channels to the Community Library.

I manually tested the changes by interacting with it in the browser, tried network throttling simulation to check the experience for users on slow networks, and tried switching the interface to Arabic to see how it handles RTL languages.

Detailed changes

• Implemented the SubmitToCommunityLibrarySidePanel component
• Added a "Share" dropdown in TreeViewBase
• Added addPublishedData method to the existing Channel resource
• Added a new CommunityLibrarySubmission resource
• Added constants for Community Library submission statuses
• Added shared string for Community Library submission statuses
• Moved a configured i18n-iso-countries instance to a separate util file to allow reuse outside CountryField
• Added a fullWidth prop to CountryField to allow stretching the whole parent container width
• Moved metadata strings from a mixins file to a separate shared strings file
• Moved metadata translation logic from a mixin to a standalone translateMetadataString util function (motivated by making it easier to use this logic from components using the composition API, where using mixins is not practical)

References

Resolves #5262.

Reviewer guidance

The UX of the panel when it is loading data it needs to display was invented by me and needs to be especially checked from a design perspective. Also, warnings for when the channel is already public or when the latest published version of the channel already has a submission to the community library were not present in the initial design and should be especially checked.

Unresolved questions

• The KTextbox component from KDS does not display correctly when an RTL language is used

• The categories cannot be changed in the submission form, but are allowed to be changed on the backend; the backend implementation should be changed so that this is not supported

[AlexVelezLl]

Thanks @Jakoma02! Code changes looks good! I have left some nitpick comments, we can handle them in the scope of this PR if you would have some time to take a look, otherwise no worries! We can open follow up issues for them 👐.

[AlexVelezLl]

(Nitpick) I think it'd be better to test these messages with the data-test property, as we may not want the $trs objects being used within the component (see below).

[Jakoma02]

I changed these to using the functions from the strings file for now in 608d3b9. I decided to check the strings content instead of something like data-test because a single box component is used for all the states, and only its contents (like the strings) are dynamically changed. It is done this way because the box is first shown as loading while the data is not available yet, and then the dynamic content appears inside it.

For this reason, it is not possible to set the data-test attribute directly on the box component in the template. I could dynamically assign the data-test similarly to how I assign the texts, but the tests would then test just whether the data-test attribute is assigned correctly and not whether the correct text is indeed displayed.

Do you think the current solution is still inappropriate after using the functions for the translated strings instead of the $tr method on the component instance? If so, do you have a better approach in mind?

[AlexVelezLl]

Sounds good to me!

[AlexVelezLl]

Adding these two text variables like this will end up being in a single paragraph with two sentences separated by a space. But this isn't always correct for i18n, as some languages may have other characters to separate sentences in the same paragraph. The usual suggestion is to have all sentences in a single string, so it can be translated correctly.

So, for this, it'b be better to have just one string in a single span/div node, and set a data-test property for that node so we can assert against that property in the tests.

[Jakoma02]

In fact, the info-box-secondary-text has display: block applied, so the secondary text will be separated by a line break. This is aligned with the messages appearing on separate lines in the Figma design. I cannot find the SO answer that inspired me to do this, but this one recommends this approach as well.

Do you think this is still an issue if the sentences are not separated by a space?

PS: I was not aware that there were languages with other sentence separators than a space, thanks for the link!

[AlexVelezLl]

Oh didn't see the display block. If it is a block element, then we can perhaps change the span node to be a div! Span elements are inline by definition, so perhaps a div or a p elements would communicate better the intention of the separation of these sentences.

[Jakoma02]

Changed to use a div in 6cb77b5.

[AlexVelezLl]

For this side panel I think we don't need to load all previous submissions, right? As this viewset supports pagination, we can probably get some pagination/ordering params as arguments of this composable and load just the last submission in the side panel.

[Jakoma02]

You are of course correct, I am not sure why I did not think of that. Changed in 2709c46.

[AlexVelezLl]

Could we rename this file to useFetch to match the useFetch composable? And since this is a more general composable, we can probably place this in a contentcuration/frontend/channelEdit/composables folder?

[Jakoma02]

Done in 91850d6.

[AlexVelezLl]

(Nitpick) It was a bit difficult for me to fully understand the intention of this "none" magic string, and the reason for it I think was because we are trying to represent too many things with the same variable:

• The variable is null if the data hasn't been loaded yet.
• The variable is "none" if the data has loaded and we have confirmed that there isn't any previous submission.
• In any other case it represents the actual status of the latest submission.

I think a strategy that can declare more verbosely what the intention of the component would be to directly check the variable submissionsAreFinished in the template. So we would have

              <template v-if="submissionsAreFinished && !latestSubmissionStatus">
                 ...
              </template>
             ...
            <template #chip>
              <StatusChip
                v-if="submissionsAreFinished && latestSubmissionStatus"
             ...

and is a bit easier to read that the intention is to show these nodes just once the load of the submissions has finished. Another option would be to create a computed property for checking this. Let me know what you think!

[Jakoma02]

The reason for using "none" in the case where there is no previous submission was to allow using it as a key of an object, as null is not a valid key. But I agree that this is confusing. I will try to structure this better.

[AlexVelezLl]

Yes, I imagined it was because of that. Perhaps using just a computed property to return the needed values are a more readable option without using the "none" magic string.

[Jakoma02]

Resolved in bf8a914 by using a computed property instead of indexing an object.

[AlexVelezLl]

// NOTE: Uses of translated strings inside setup are not picked up by ESLint

This is one of the reasons why we now use strings files for strings 😅.

One of the requirements described in the issue is not being met:

All new strings should be created in the communityChannelsStrings file.

This is the new standard that we are using for user-facing strings to reuse these strings across multiple components in the same feature, and allows better organization for the translation process.

[Jakoma02]

Sorry, I missed the note that the strings file should be used. Changed in 608d3b9.

[AlexVelezLl]

Doesn't seem like we are using the conceptual difference between undefined and null anywhere in the setup method, so this perhaps can be rewritten to

  if (!submissionsData.value?.length) return null;

[Jakoma02]

This is likely not relevant anymore due to changes in 2709c46.

However, I would still like to explain my thought process here, because I use similar reasoning at other places as well. Even though the undefined and null are treated the same way in the code, they convey different semantic meanings: the null value means that there is no latest submission, while undefined means the latest submission is not known yet. I see value in distinguishing these mainly during debugging issues. If I print out latestSubmission.value, I will not be misled by it having a null value, that there is no latest submission when there was an error in retrieving it, or conversely.

Do you think there is a mistake in this thought process, and should I approach these situations differently?

[AlexVelezLl]

It's a matter of preferences, and definitely, there isn't a correct answer! We can live with the conceptual difference between undefined and null represented in this computed property, just in case it's needed.

I've flagged it just because we should also consider how future readers of this code will react when they encounter it. e.g., suppose I have no context for this code and read these conditions. In that case, I would think that we are explicitly returning these two different values because that difference will somehow impact the final result in the UI, and would try to look at the setup method or any other child component until I finally discover that the different values does not impact the final result in the UI, these can perhaps take couple of minutes that we would have saved if we would have declare the intention better just returning the same value for both cases!

But... It's just an assumption! There's no need to change it, but it's worth also considering this aspect of "what will future maintainers think when they read my code?"

[AlexVelezLl]

(A reason why a future maintainer would like to check the references of this property instead of just assuming that they are intentionally set to undefined or null just because this is the correct semantic meaning is that in general the null vs undefined seems to be a general debate, and even standards api's have not been using them consistently, so people may not be fully used to see code that differentiates the actual semantic meaning.)

[Jakoma02]

Thank you for elaborating, this is a good point. I was not aware that the distinction between null and undefined was not universally accepted. I added comments clarifying the intention behind the distinction in 86bd2fc.

[AlexVelezLl]

Thanks a lot @Jakoma02!

[AlexVelezLl]

Would be great if we have some comments here regarding the logic behind using a delay before actually sending the submission.

[Jakoma02]

Added a simple comment in b200765.

[AlexVelezLl]

Seems like we have some unwrapped strings here.

[Jakoma02]

You are right, I missed these. Fixed in 608d3b9.

[AlexVelezLl]

Thanks a lot for all your hard work, @Jakoma02! And thank you for all your contributions to Studio! It has been a pleasure working with you on this project over the past few months! LGTM!