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.

Sign up for GitHub

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

Jump to bottom

Escapes Html #7542

Merged
merged 2 commits into from
Sep 26, 2024
Merged

Escapes Html #7542

merged 2 commits into from
Sep 26, 2024

Conversation

VikalpRusia
Copy link
Contributor

@VikalpRusia VikalpRusia commented Sep 25, 2024 •
edited
Loading

Changes

Escape the snapshot name in result banner

Why?

Causing unintended interaction with the UI elements.

How

Escape the name of the created snapshot in the banner displayed in the snapshot list to avoid interpreting it as HTML.

Details

Referring to this comment -
#7508 (comment)

Changes

  • reused the escapeHtml function in utils/string file

Screenshots

Screenshot 2024-09-26 at 1 09 20 AM Screenshot 2024-09-26 at 1 09 27 AM

@VikalpRusia VikalpRusia marked this pull request as ready for review September 25, 2024 09:09
@mook-as mook-as self-requested a review September 25, 2024 16:50
@mook-as
Copy link
Contributor

mook-as commented Sep 25, 2024

I am confused. Per the screenshot:
Screenshot 2024-09-18 at 12 55 47 PM

It's the notification message (the green bar) that's having an error, not the card. But this PR is changing SnapshotCard.vue which is the card (where everything is fine)?

@VikalpRusia
Copy link
Contributor Author

@mook-as, sorry misunderstood it.

Will fix it

@VikalpRusia VikalpRusia marked this pull request as draft September 25, 2024 18:57
@VikalpRusia
Copy link
Contributor Author

VikalpRusia commented Sep 25, 2024 •
edited
Loading

@mook-as can you re-review, fixed it

@VikalpRusia VikalpRusia marked this pull request as ready for review September 25, 2024 20:05
mook-as
mook-as requested changes Sep 25, 2024
View reviewed changes
Copy link
Contributor

@mook-as mook-as left a comment

Choose a reason for hiding this comment

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

Since this is a UI PR, please attach a screenshot of the result.

pkg/rancher-desktop/components/Snapshots.vue Outdated Show resolved Hide resolved
pkg/rancher-desktop/components/Snapshots.vue Outdated Show resolved Hide resolved
@mook-as
Copy link
Contributor

mook-as commented Sep 25, 2024

FWIW: related, but not for this PR:
image
The status dialog has the same issue. (But yeah, don't worry about it for now, let's just get this merged first.)

@VikalpRusia
escaped-html-notifcation
4762641 
Signed-off-by: Vikalp Rusia <vikalprusia@gmail.com>
@VikalpRusia
Copy link
Contributor Author

Since this is a UI PR, please attach a screenshot of the result.

edited the description of PR

@VikalpRusia
Copy link
Contributor Author

@mook-as re-requested your review
:)

@VikalpRusia VikalpRusia mentioned this pull request Sep 26, 2024
Merged
mook-as
mook-as previously approved these changes Sep 26, 2024
View reviewed changes
Copy link
Contributor

@mook-as mook-as left a comment

Choose a reason for hiding this comment

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

Just trivial stuff that shouldn't block merging for a new contributor 🎉

pkg/rancher-desktop/components/Snapshots.vue Outdated Show resolved Hide resolved
pkg/rancher-desktop/components/Snapshots.vue Outdated Show resolved Hide resolved
@VikalpRusia
Copy link
Contributor Author

Just trivial stuff that shouldn't block merging for a new contributor 🎉

@mook-as refactored code as per your suggestion can you please re-review
:)

mook-as
mook-as previously approved these changes Sep 26, 2024
View reviewed changes
@VikalpRusia
refactored-code
2433160 
Signed-off-by: Vikalp Rusia <vikalprusia@gmail.com>
@mook-as
Copy link
Contributor

mook-as commented Sep 26, 2024

FWIW, I like having commit messages that explain what's going on; for example 63edddf has a commit message longer than the change :) But again that's something that I wouldn't expect of a new contributor (and doesn't always happen with us either).

For this one, I would imagine something in the lines of:

Escape the snapshot name in result banner

Escape the name of the created snapshot in the banner displayed in the snapshot list to avoid interpreting it as HTML and causing unintended interaction with the UI elements.

There's some advice in the git manual on the format of commit messages; but reading that isn't mandatory or anything.

Looks like there are some linting errors; please run yarn lint to auto-fix what it can (and report the other things you need to manually handle).

@mook-as mook-as merged commit 8af4e1b into rancher-sandbox:main Sep 26, 2024
17 checks passed
@VikalpRusia VikalpRusia deleted the escape-html branch September 26, 2024 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mook-as mook-as mook-as approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@VikalpRusia @mook-as