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

Add ability to show component in alert #1118

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

matthew-white
Copy link
Member

Right now, alerts can only show text. However, I've long wanted to lift that restriction in order to make it possible to show other content. I think lifting that restriction would make it easier to solve issues like getodk/central#775.

What got me thinking about this again was #1075. There, the alert lists one or more entity properties. It would've been nice if we could have shown a <ul>, but because the alert had to be text, that wasn't possible. Instead, we added bullet points to the text. With the change in this PR, it's now possible to show a <ul> in the alert in that case. That's done by passing a component to the alert.

What has been done to verify that this works as intended?

I've updated existing tests. If this approach looks good, I'll add some new tests as well.

Why is this the best possible solution? Were any other approaches considered?

The approach in this PR is to pass a component to the alert. I also thought about passing HTML or Markdown. However, a component gives us the most flexibility. It also gives us an easy way to escape user content and not render user content as HTML.

Before submitting this PR, please make sure you have:

  • run npm run test and npm run lint and confirmed all checks still pass OR confirm CircleCI build passes
  • verified that any code or assets from external sources are properly credited in comments or that everything is internally sourced


<script setup>
defineOptions({
name: 'Alert40917'
Copy link
Member Author

Choose a reason for hiding this comment

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

I tried naming this Alert409_17, but the linter complained whenever I imported it:

Identifier 'Alert409_17' is not in camel case

@matthew-white matthew-white marked this pull request as draft December 20, 2024 16:41
@matthew-white
Copy link
Member Author

@sadiqkhoja, would you mind taking a quick first look at the approach in this PR? If it looks good, I can add more tests and get it ready for a full review.

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.

1 participant