Skip to content
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

HARVESTER: Fix Harvester image issues #6502

Merged
merged 4 commits into from
Jul 29, 2022
Merged

Conversation

n313893254
Copy link
Contributor

@n313893254 n313893254 commented Jul 26, 2022

Summary

Fixes Harvester image issues

Occurred changes and/or fixed issues

Technical notes summary

Areas or cases that should be tested

Areas which could experience regressions

Screenshot/Video

@n313893254 n313893254 marked this pull request as ready for review July 26, 2022 15:05
@n313893254 n313893254 changed the title Fix Harvester image issues HARVESTER: Fix Harvester image issues Jul 26, 2022
@mantis-toboggan-md mantis-toboggan-md self-requested a review July 26, 2022 17:40
@mantis-toboggan-md mantis-toboggan-md added this to the v2.6.7 milestone Jul 26, 2022
@@ -159,6 +159,7 @@ export default {
:localized-label="true"
:mode="mode"
:options="enumOptions"
@keydown.native.enter.prevent="()=>{}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should try to solve this in a more general way so we don't accidentally cause this bug on other pages in the future. We can just put @submit.prevent on the form element here: https://github.com/rancher/dashboard/blob/master/shell/components/CruResource.vue#L344

This only happens with <form> that have only one input element, so I do not expect this change to cause problems elsewhere. https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#implicit-submission

Copy link
Contributor Author

@n313893254 n313893254 Jul 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, added the @submit.prevent for CruResource form.

@n313893254 n313893254 marked this pull request as draft July 27, 2022 06:24
@n313893254 n313893254 marked this pull request as ready for review July 27, 2022 08:14
@n313893254
Copy link
Contributor Author

@mantis-toboggan-md Thanks for your review. Updated the PR per the comments.

@@ -384,7 +384,7 @@ export default {
</nuxt-link>
<span v-else>{{ parent.displayName }}:</span>
<span v-if="value.detailPageHeaderActionOverride && value.detailPageHeaderActionOverride(realMode)">{{ value.detailPageHeaderActionOverride(realMode) }}</span>
<t v-else :k="'resourceDetail.header.' + realMode" :subtype="resourceSubtype" :name="displayName" />
<t v-else :k="'resourceDetail.header.' + realMode" :subtype="resourceSubtype" :name="displayName" :escapehtml="false" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you have to add a prop to the i18n mixin to render the name in a way that matches the list view, as required in the ticket harvester/harvester#2563. This should render the same way:

<span v-else>
   {{ t(`resourceDetail.header.${realMode}`, {subtype: resourceSubtype, name:displayName}, {raw:true}) }}
</span>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mantis-toboggan-md This is very strange.

  1. The raw parameter indicates that we want it to convert the html correctly. (But here we don't want it to convert, the html stays as it is),
  2. {raw: true} should be changed to true
    Vue.prototype.t = function(key, args, raw) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The question here is, how should I interpret the raw parameter inside Vue.prototype.t.
If raw here is the same as raw in Vue.component, then I think it makes sense to add escapehtml prop,

Copy link
Member

@mantis-toboggan-md mantis-toboggan-md Jul 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{raw: true} should be changed to true,

You're absolutely right, I apologize for being confusing. The way I wrote it is wrong but "worked" because of how this if statement is written:

if ( raw ) {
return out;
} else {
return escapeHtml(out);
}

My solution works because Vue interprets expressions in {{}} as plain text. So these two examples do not render the same way:

<t v-else :k="'resourceDetail.header.' + realMode" :subtype="resourceSubtype" :name="displayName" raw />
<span v-else>
  {{ t(`resourceDetail.header.${realMode}`, {subtype:resourceSubtype, name: displayName}, true) }}
</span>

<t raw> will not call escapeHtml on the translation string, and will set the element's innerHTML to include raw (aka unescaped) html. That would render the example text <strong><em>something_interesting</em></strong> as something_interesting

if ( this.raw ) {
return h(
this.tag,
{ domProps: { innerHTML: msg } }
);
} else {

When we use t(<translation key>, <translation args>, true) inside {{}}, t returns a string with HTML unescaped, but the html is rendered as plain text by vue anyway. I think this is also why, in the original issue, we saw html tags as plain text in the list view:
<n-link v-if="to" :to="to">
{{ value }}
</n-link>

value in the issue example is "<strong><em>something_interesting</em></strong>" but then, because it is in {{}}, it is rendered as plain text.

I think needing to render a translation containing user input is rare, so I didn't want to add a prop to i18n mixin to handle this one situation. Having said that I understand that my solution relies on a detail of Vue implementation instead of something we have explicitly written in the codebase, so if the code is not clear enough and you want to keep the prop I'm okay with that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think it might be good to keep this prop.

@mantis-toboggan-md mantis-toboggan-md merged commit 3878303 into rancher:master Jul 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants