-
Notifications
You must be signed in to change notification settings - Fork 16
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
SEAB-5820: Mastodon feed widget #1837
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #1837 +/- ##
===========================================
- Coverage 40.70% 40.41% -0.30%
===========================================
Files 364 364
Lines 11252 11261 +9
Branches 2866 2888 +22
===========================================
- Hits 4580 4551 -29
- Misses 4376 4411 +35
- Partials 2296 2299 +3
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small comment, looks good in terms of screenshot.
Thanks for fixing this up!
(and thanks Charles for deciphering what happened)
src/app/footer/footer.component.html
Outdated
class="footer-link" | ||
fxLayoutGap="0.5rem" | ||
> | ||
<img class="site-icons-tab" src="../../assets/images/dockstore/mastodon.svg" alt="small Twitter logo" /><small |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alt text needs update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alt text needs update
Comment still applicable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I
- Go to home page, Mastodon feed displays
- Click on About
- Click to go back to home; Mastodon feed is not there.
I think this is best solved by creating an Angular component; see my inline comment.
The major downside is we risk getting out of date with the original source for this, but I think it's worth it.
postDate: string; | ||
postContent: string; | ||
spoilerText: string; | ||
mediaContent: any[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
had to use any[]
because we are getting these data from the mastodon api
spoilerText: string; | ||
mediaContent: any[]; | ||
previewLink: string; | ||
poll: any[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
@denis-yuen tagging for rereview |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First, thanks for doing all this work! You went beyond what I was expecting, which just a thin wrapper around the existing Mastodon file, as a component.
Two thoughts beyond the more specific comments I left:
- It would be good if there were a source code comment linking to the code you adopted your code from, in case it gets updated and we need to port it over.
- I feel like the UI needs a box and/or title around it. It's not obvious to me, that it's a Mastodon feed, unless you happen to notice the "See more posts at Mastodon" link at the bottom of the component.
src/app/footer/footer.component.html
Outdated
class="footer-link" | ||
fxLayoutGap="0.5rem" | ||
> | ||
<img class="site-icons-tab" src="../../assets/images/dockstore/mastodon.svg" alt="small Twitter logo" /><small |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alt text needs update
Comment still applicable.
updated the UI by wrapping it with a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updates look good, cannot technically approve, but otherwise approved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! My main question is whether we should show more than 2 posts in the mastodon timeline. Maybe 5 so the user can scroll?
SonarCloud Quality Gate failed. 0 Bugs 14.2% Coverage Catch issues before they fail your Quality Gate with our IDE extension SonarLint |
</div> | ||
<mat-divider></mat-divider> | ||
<div class="mt-footer"> | ||
<a [href]="mastodonDockstoreLink" class="btn" target="_blank" rel="nofollow noopener noreferrer"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btn
class doesn't exist
Description
This PR adds a the Mastodon Dockstore feed where the old twitter feed used to be.
Homepage:
Dashboard
Review Instructions
On qa, verify that the widget loads properly on the homepage and dashboard. Also, the new tests should pass.
Issue
https://ucsc-cgl.atlassian.net/browse/SEAB-5820
Security
If there are any concerns that require extra attention from the security team, highlight them here.
Please make sure that you've checked the following before submitting your pull request. Thanks!
npm run build
markdown-wrapper
component, which does extra sanitizationnpm audit
and ensure you are not introducing new vulnerabilities