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

refactor(v2): merge linkify function used in blog and docs and align properties #4402

Merged
merged 4 commits into from
Mar 12, 2021

Conversation

armano2
Copy link
Contributor

@armano2 armano2 commented Mar 11, 2021

Motivation

This PR is a continuation of work done in #4391 that aligns names of properties,

i'm unsure what is a politic here on changes, that's why i prepared 2 separate PR's for this

Have you read the Contributing Guidelines on pull requests?

yes

Test Plan

there is no change in functionality and all unit test should pass, linking still should work in docs and in blog,

it can be tested by clicking in one of links in docs, eg.
https://deploy-preview-4391--docusaurus-2.netlify.app/classic/docs/contributing#get-involved

Related PRs

#4391

@netlify
Copy link

netlify bot commented Mar 11, 2021

[V1] Deploy preview success

Built with commit 01a6fae

https://deploy-preview-4402--docusaurus-1.netlify.app

@armano2 armano2 changed the title refactor(v2): merge linkify function used in blog and docs and align properties refactor(v2): [alernative] merge linkify function used in blog and docs and align properties Mar 11, 2021
@armano2 armano2 changed the title refactor(v2): [alernative] merge linkify function used in blog and docs and align properties refactor(v2): merge linkify function used in blog and docs and align properties Mar 11, 2021
@netlify
Copy link

netlify bot commented Mar 11, 2021

Deploy preview for docusaurus-2 ready!

Built with commit 01a6fae

https://deploy-preview-4402--docusaurus-2.netlify.app

@armano2
Copy link
Contributor Author

armano2 commented Mar 11, 2021

Lighthouse failed for unknown reason

@slorber
Copy link
Collaborator

slorber commented Mar 12, 2021

Thanks, this looks like a nice code dedup refactor that I wanted to do for a while :)

I'm going to tweak some minor things before merging

@github-actions
Copy link

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟢 Performance 94
🟢 Accessibility 96
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 95

Lighthouse ran on https://deploy-preview-4402--docusaurus-2.netlify.app/classic/

@slorber
Copy link
Collaborator

slorber commented Mar 12, 2021

Basically what I did was simplify the signature of the replaceMarkdownLink function, remove the generics and return broken links instead of using a callback (will be simpler to test).

image

BTW we have tests in docs and blog, but if you want to add more isolated tests for this newly extracted method that could be useful ;)

This was referenced Mar 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: polish This PR adds a very minor behavior improvement that users will enjoy.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants