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

Fix resource_icon with component or manifest nil #28

Merged
merged 3 commits into from
Nov 16, 2022
Merged

Fix resource_icon with component or manifest nil #28

merged 3 commits into from
Nov 16, 2022

Conversation

fblupi
Copy link

@fblupi fblupi commented Nov 15, 2022

🎩 What? Why?

Fix resource_icon when component or manifest is nil

Testing

Generate a notification with a resource with a nil component and go to /notifications

♥️ Thank you!

@fblupi fblupi requested a review from davidbeig November 15, 2022 08:34
davidbeig
davidbeig reviewed Nov 16, 2022
View reviewed changes
Copy link

@davidbeig davidbeig left a comment

Choose a reason for hiding this comment

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

Hi!

I have tested this PR and found that it throws this error

imatge

Maybe we should check if manifest responds to icon, if it does not then it will render "question-mark" as icon.

What do you think? Did this error appear to you? Is there anything that I'm missing?

@fblupi fblupi requested a review from davidbeig November 16, 2022 10:44
@fblupi
Copy link
Author

fblupi commented Nov 16, 2022

Hi!

I have tested this PR and found that it throws this error

imatge

Maybe we should check if manifest responds to icon, if it does not then it will render "question-mark" as icon.

What do you think? Did this error appear to you? Is there anything that I'm missing?

Already fixed with a safe navigation operator

davidbeig
davidbeig approved these changes Nov 16, 2022
View reviewed changes
Copy link

@davidbeig davidbeig left a comment

Choose a reason for hiding this comment

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

It works perfect rn!

Thanks for the good work!

@fblupi fblupi merged commit f4cf37e into release/0.26-stable-bcn Nov 16, 2022
@fblupi fblupi deleted the fix/resource-icon-with-component-or-manifest-nil branch November 16, 2022 12:44
@fblupi fblupi mentioned this pull request Nov 23, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidbeig davidbeig davidbeig 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
@fblupi @davidbeig