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

✨ attempt to introduce html in notifications #359

Closed
wants to merge 2 commits into from
Closed

✨ attempt to introduce html in notifications #359

wants to merge 2 commits into from

Conversation

acidjazz
Copy link
Contributor

image addresses issue #358

@nuxt-studio
Copy link

nuxt-studio bot commented Jun 28, 2023

Live Preview ready!

Name Edit Preview Latest Commit
ui Edit on Studio ↗︎ View Live Preview 6a7c8ed

@vercel
Copy link

vercel bot commented Jun 28, 2023

@acidjazz is attempting to deploy a commit to the Nuxt Labs Team on Vercel.

A member of the Team first needs to authorize it.

@Haythamasalama
Copy link
Contributor

What about using slot rather than v-html ?

@acidjazz
Copy link
Contributor Author

What about using slot rather than v-html ?

Can I get an example? I'd like to be able to make this programmatic

Comment on lines +11 to +12
<p :class="ui.title" v-html="title" />
<p v-if="description" :class="ui.description" v-html="description" />
Copy link
Contributor

@Haythamasalama Haythamasalama Jun 28, 2023

Choose a reason for hiding this comment

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

@acidjazz that's what I mean

<slot>
  <p :class="ui.title">
    {{ title }}
  </p>

  <p v-if="description" :class="ui.description">
    {{ description }}
  </p>
</slot>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Haythamasalama I see, thanks for the example, how would this work programmatically?

Copy link
Contributor

Choose a reason for hiding this comment

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

<UNotification>
  <p>
   // your title 
  </p>
  <p>
   // your description  
  </p>
</UNotification>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no I mean using the composable useToast().add here

@jaybharadia
Copy link
Contributor

Hello @acidjazz , Thanks for contributing.

v-html ( Security Concern )

Vue automatically escapes HTML content, preventing you from accidentally injecting executable HTML into your application. However, in cases where you know the HTML is safe, you can explicitly render HTML content

Question

Should we sanitize the html ?

Suggestions

It would be nice to see how other UI Libraries are handling this scenario.

Ending Note

What are your thoughts @benjamincanac on this ?

References

@acidjazz
Copy link
Contributor Author

acidjazz commented Jun 28, 2023

Hello @acidjazz , Thanks for contributing.

v-html ( Security Concern )

Vue automatically escapes HTML content, preventing you from accidentally injecting executable HTML into your application. However, in cases where you know the HTML is safe, you can explicitly render HTML content

Question

Should we sanitize the html ?

Suggestions

It would be nice to see how other UI Libraries are handling this scenario.

Ending Note

What are your thoughts @benjamincanac on this ?

References

Yea that's a good point, are there any general practices/suggestions/etc out there we can use to properly sanitize this?

Or maybe we can only make it v-html if we have some kind of flag/option turned on so the user is aware?

maybe like:

<u-button html />

then add to our composables:

useToast().add({title: 'test<b>ting</b>', html: true})

I can commit this change if that's agreeable, then put a notice/warning in the docs for this option

Copy link
Member

I was just thinking, wouldn't it be better to handle Markdown through MDC syntax instead? This would open way more possibilities, although it would require to add @nuxt/content as a dependency for its transformer.

@jaybharadia
Copy link
Contributor

I was just thinking, wouldn't it be better to handle Markdown through MDC syntax instead? This would open way more possibilities, although it would require to add @nuxt/content as a dependency for its transformer.

Need to explore this possibility 👍

@acidjazz
Copy link
Contributor Author

acidjazz commented Jul 5, 2023

I was just thinking, wouldn't it be better to handle Markdown through MDC syntax instead? This would open way more possibilities, although it would require to add @nuxt/content as a dependency for its transformer.

I think I'm going to attempt the html being a flag/option that they specify for it to happen, w/ the v-html warning.

I can also explore a flag/option of markdown that they specify, maybe we can have the dependency be optional based on if they turn it on somehow?

@vercel
Copy link

vercel bot commented Jul 5, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
ui ✅ Ready (Inspect) Visit Preview Jul 5, 2023 5:10pm

@benjamincanac
Copy link
Member

Here is my take on this: #431. What do you think?

@acidjazz
Copy link
Contributor Author

awesome! - i like that the control that comes w/ adding the slots in app.vue - i think for the ideas for markdown and other support that were discussed we can make separate issues

@benjamincanac
Copy link
Member

Closing in favor of #431

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.

4 participants