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

Flatten the URL list to generate valid links #101

Merged
merged 1 commit into from
Jan 18, 2022
Merged

Conversation

hellais
Copy link
Member

@hellais hellais commented Jan 18, 2022

Fixes: #100

@vercel
Copy link

vercel bot commented Jan 18, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/ooni/run/C7VE66caRcgxNFFM92GzgG8Xeh1A
✅ Preview: https://run-git-fix-ooni-run-links-ooni1.vercel.app

Copy link
Contributor

@bassosimone bassosimone left a comment

Choose a reason for hiding this comment

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

🐳

If it's not too long and tedious, I'd also probably add a test that verifies that we don't break this again, otherwise this PR is good to merge for me, thank you!

(If writing a test turns out to be indeed a long task, I think we should probably discuss about making this repository easier to test as part of a separate conversation.)

@@ -56,7 +56,7 @@ const TwitterButton = ({ universalLink }) => {
return (
<TwitterShareButton
url={universalLink}
message={intl.formatMessage({id: 'Share.Twitter.Tweet', defaultMessage: 'Run OONI Probe to test for censorship!'})}
message={intl.formatMessage({ id: 'Share.Twitter.Tweet', defaultMessage: 'Run OONI Probe to test for censorship!' })}
Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like my editor has done some automatic reformatting of several blocks. Most changes can be ignored.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

@@ -72,7 +72,7 @@ const Home = () => {

const [universalLink, embedCode] = useMemo(() => {
console.log('generating links and embed code')
const universalLink = getUniversalLink([...urls])
const universalLink = getUniversalLink(urls.map((e) => e.url))
Copy link
Member Author

Choose a reason for hiding this comment

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

The only relevant change is this line here.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@hellais
Copy link
Member Author

hellais commented Jan 18, 2022

I'd also probably add a test that verifies that we don't break this again, otherwise this PR is good to merge for me,

Unfortunately we don't currently have any testing infrastructure in place in this repo. Adding a test for it basically involves adding all the boilerplate needed to get it working in the repo.

I agree it's a good idea, but it's a larger task that I suggest we do as future work: #102

@hellais hellais merged commit 329ee0a into master Jan 18, 2022
@hellais hellais deleted the fix/ooni-run-links branch January 18, 2022 13:40
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.

Generated OONI Run links are invalid
2 participants