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
feat: add support for blogpost from other (generic) platforms #1774
Conversation
β Deploy Preview for design-insights ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
β Deploy Preview for oss-insights ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
PR Compliance Checks
Thank you for your Pull Request! We have run several checks on this pull request in order to make sure it's suitable for merging into this project. The results are listed in the following section.
Watched Files
This pull request modifies specific files that require careful review by the maintainers.
Files Matched
- npm-shrinkwrap.json
- package.json
Hey @babblebey, would you mind fixing up the merge conflict before we proceed to reviewing this pr? |
We've gotten some feedback that people want this feature: any updates here? Or if it's more or less ready to go we could merge beta onto @babblebey's branch |
Hey @jpmcb, it's pretty much ready, I just gotta fix the conflicts π Will do shortly π€πΎ |
β Deploy Preview for oss-insights failed.
|
β¦s the primary CTA (open-sauced#2334)
## [1.80.0-beta.1](open-sauced/app@v1.79.3-beta.1...v1.80.0-beta.1) (2023-12-13) ### π Bug Fixes * increase z-index of show latest pull request button ([d0b7722](open-sauced@d0b7722)) * now the add more button for adding more contributors to a list is the primary CTA ([09df01c](open-sauced@09df01c)) * now the add more button for adding more contributors to a list is the primary CTA ([open-sauced#2334](open-sauced#2334)) ([ef070f2](open-sauced@ef070f2)) * pull request button shows PRs now on a contributor card ([open-sauced#2277](open-sauced#2277)) ([a87bdff](open-sauced@a87bdff)) ### π Features * use GitHub API for user search box ([open-sauced#2331](open-sauced#2331)) ([152b933](open-sauced@152b933))
β¦ list is loading
β¦contributors" while the contributor list is loading. (open-sauced#2339)
β¦mpty className props
Signed-off-by: John McBride <john@opensauced.pizza>
## [1.85.0-beta.4](open-sauced/app@v1.85.0-beta.3...v1.85.0-beta.4) (2024-01-26) ### π Bug Fixes * add a link to the workspace settings page in the app sidebar ([open-sauced#2541](open-sauced#2541)) ([b598c0f](open-sauced@b598c0f)) * Bust app cache for v2 API changes ([open-sauced#2545](open-sauced#2545)) ([07b2f1d](open-sauced@07b2f1d))
β¦r remove as expected (open-sauced#2544)
Hey @babblebey I'm going to move this to draft mode until you have time to work on it. |
## [1.85.0-beta.5](open-sauced/app@v1.85.0-beta.4...v1.85.0-beta.5) (2024-01-29) ### π Bug Fixes * hydration issue for suggested repo list component ([open-sauced#2535](open-sauced#2535)) ([31f8c37](open-sauced@31f8c37)) * now checkboxes in the search for tracked repositories flow add or remove as expected ([open-sauced#2544](open-sauced#2544)) ([390c760](open-sauced@390c760)) ### π Features * created the repository stat card component ([open-sauced#2546](open-sauced#2546)) ([bc35e83](open-sauced@bc35e83))
β¦bey/insights into feat/generic-blog-support
Hey apologies @BekahHW, this proved super weird of a conflict to resolve, after many attempts at resolving, It kept coming back asking that I resolve over and over again. I ended up moving for rebasing and that brought in all the 390 changes π PR still works as it should for sure. I don't know exactly what impact the 390 changes would make on merge whenever it gets there, but I do recognize how uneasy it's become to review this PR now.... So kindly guide me here, will you say I keep this PR as is or would you prefer that I repatch this feature in another new PR that can remove the burden of the out-of-context 300+ file changes!? |
At this point this is not reviewable and the PR is getting stale. My recommendation is create a new PR from a new branch from beta and cherry picking the commits you actually need. Going to close this PR since this will require git surgery or some serious fixing to get back to unity. |
I should patch this in less than 24h now, and I just mistakenly clicked "Delete Branch" π€¦πΎ |
Description
This Pull Request adds support for blogpost from other platforms in highlights. Leverages the
open-graph
node library, fetches the open graph data from the given blogposturl
provided ashighlightLink
and uses theimage
property on the data as theblogspot
highlight type social image when posting new highlights.Implementation Caveat π€
This is specifically because in order to consume the auto-summarize function, the
generateDevToBlogHighlightSummary
(which is an auto-summary function for theDevTo
blogpost) takes two(2) arguments namely theblogTitle
andblogMarkdown
(which is the blog post content). This implementation uses theopen-graph
to fetch meta data which satisfies only theblogTitle
. There isn't a straight-forward or consistent or lightweight implementation (aside scrapping π€«) to access the body content of this generic blog post.In the absence of the body content of this blog post, I hence settled for implementing a notice of
no-support for auto-summarize
for theauto-summarize
feature on the generic blog post for now.Changes Made
DevTo
related components and functions, to reflectDevTo***
in its naming to make them more specific todev.to
blog post typefetchOGData
endpoint in the/api
route which fetchesopen-graph
data for specified urlGenericBlogOpenGraphImage
component which consumes thefetchOGData
endpoint, builds and sends response image as returned component node.isValidURL
helper function to validate url input in thehighlightLink
field on highlight input form.GenericBlogOpenGraphImage
component andisValidURL
function in appropriate base components.toast
in attempt to add notification forauto-summarize
not being supported for blogpost from these generic platform because we do not currently have any means of accessing the exact blog post body content which is one of the required parameter/content for auto-summarize.π
What type of PR is this? (check all applicable)
Related Tickets & Documents
Fixes #1656
Mobile & Desktop Screenshots/Recordings
Writing a (Generic) Blogpost highlight type
screencast-localhost_3000-2023.10.15-04_00_37.webm
Blogpost (without tagged repos)
Blogpost (with tagged repos)
Demo: Toast for
Not Supported Message for Auto-Summarize
Added tests?
Added to documentation?
[optional] Are there any post-deployment tasks we need to perform?
This PR adds a new package
open-graph
[optional] What gif best describes this PR or how it makes you feel?