Skip to content

Conversation

@SebastianSupreme
Copy link
Contributor

@SebastianSupreme SebastianSupreme commented Jan 29, 2023

Describe your changes

The two plaintext sections have been placed incorrectly.
Now that I switched them, the whole paragraph makes sense again.

Files changed

docs/how-to/address-ipfs-on-web.md

Does this update depend on any other PRs?

No.

The two plaintext sections have been placed incorrectly.
Now that I switched them, the whole paragraph makes sense again.
@welcome
Copy link

welcome bot commented Jan 29, 2023

Thank you for submitting this PR!
A maintainer will be here shortly to review it.
We are super grateful, but we are also overloaded! Help us by making sure that:

  • The context for this PR is clear, with relevant discussion, decisions and stakeholders linked/mentioned.
  • Your contribution itself is clear (grammar and spelling checked, any code blocks checked and commented) and in its best form. Follow the docs contribution guidelines if they apply.

Getting other community members to do a review would be great help too on complex PRs (you can ask in the chats/forums). If you are unsure about something, just leave us a comment.
Next steps:

  • A maintainer will triage and assign priority to this PR, commenting on any missing things and potentially assigning a reviewer for high priority items.
  • The PR gets reviews, discussed and approvals as needed.
  • The PR is merged by maintainers when it has been approved and comments addressed.

We currently aim to provide initial feedback/triaging within two business days. Please keep an eye on any labelling actions, as these will indicate priorities and status of your contribution.
We are very grateful for your contribution!

Copy link
Contributor

@TMoMoreau TMoMoreau left a comment

Choose a reason for hiding this comment

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

This makes sense to me.

@SebastianSupreme
Copy link
Contributor Author

SebastianSupreme commented Feb 3, 2023

Sorry, accidentally closed this a few minutes ago. 😅
I'm glad you reopened it. 🫡

@ElPaisano ElPaisano self-requested a review February 3, 2023 01:01
Copy link
Contributor

@ElPaisano ElPaisano left a comment

Choose a reason for hiding this comment

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

Hi @SebastianSupreme thank you for taking the time read our docs and open this pull request.

After reviewing this PR / merging in the latest from main, I would propose that a change before this is merged: 51182c1. I do understand the confusion here that prompted this PR, but I do not think the plaintext sections should be switched - I think that they were in the correct location, but the writing and section formatting / structure made it unclear. Your PR indicates that this page needs improvement. So, in the commit above, I regrouped, rewrote and rearranged some of the content in the section to hopefully better clarify the different ways to construct path URLs given an IPFS id. I may be incorrect here, but this is the way I am interpreting this section.

The draft is here https://bafybeig5irfij4ehycf2sr5mslnfpsp5th73vmj5kpfa3k4oxokcejbigq.on.fleek.co/how-to/address-ipfs-on-web/#path-gateway

Hopefully this addresses the issue here. Thank you for the PR and the feedback, and apologies for the confusion here.

This example somehow ended up in the ipns section which needs a new example now. 🤷‍♂️
@ElPaisano ElPaisano requested review from 2color and TMoMoreau February 3, 2023 01:02
@SebastianSupreme
Copy link
Contributor Author

@ElPaisano Thank you for your time to review my PR.
I've discovered a minor mistake in your new commit where you put a misleading example in the ipns section.
I put it back in its rightful place though I'd appreciate it if you could add a proper ipns example to the respective section because now it's missing one.

@ElPaisano
Copy link
Contributor

@2color could you take a quick look at this draft / PR and let us know if you think the latest updates are the correct resolution?

@TMoMoreau
Copy link
Contributor

@ElPaisano This was a misunderstanding of the doc on my part, after reading the page again and getting a better understanding of the content, your changes make sense. I'm going to do one more quick pass and approve again.

@SebastianSupreme
Copy link
Contributor Author

SebastianSupreme commented Feb 6, 2023

@ElPaisano @TMoMoreau
Can we merge this PR now?
The requested changes have been implemented.

@SebastianSupreme SebastianSupreme requested review from ElPaisano and removed request for 2color February 7, 2023 01:24
@ElPaisano ElPaisano merged commit 597516e into ipfs:main Feb 7, 2023
@SebastianSupreme SebastianSupreme deleted the patch-1 branch February 7, 2023 22:51
@ElPaisano ElPaisano self-assigned this Feb 8, 2023
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.

3 participants