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

Updating to Edit this page on Github #582

Merged
merged 8 commits into from
Apr 15, 2024

Conversation

Sahil-Shadwal
Copy link
Contributor

@Sahil-Shadwal Sahil-Shadwal commented Mar 22, 2024

What kind of change does this PR introduce?
This PR updates the button text message from Make a contribution to --> Edit this page on github.
And also updates the link so it would point to the website github page and points to the code of the page you clicked the button from.

Issue Number:

Screenshots/videos:

If relevant, did you update the documentation?
NO

Summary
This is just simple button update so it would say right thing and point to right link.

Does this PR introduce a breaking change?

@Sahil-Shadwal Sahil-Shadwal requested a review from a team as a code owner March 22, 2024 14:14
@Sahil-Shadwal
Copy link
Contributor Author

The Edit this page button takes the user to component that renders the page.
As for pointing the link to the markdown, the markdown files don't have consistent names. So doing that means adding a lot of static code . What do you suggest @benjagm , how we should solve this ?

@benjagm
Copy link
Collaborator

benjagm commented Mar 22, 2024

The Edit this page button takes the user to component that renders the page. As for pointing the link to the markdown, the markdown files don't have consistent names. So doing that means adding a lot of static code . What do you suggest @benjagm , how we should solve this ?

You can edit all the tsx pages that are using a markdown file to define a variable with the markdown file name. This new variable will serve the current purpose on those pages but also will be used to call the feedback component to generate the link. In cases when there is no markdown file the file to edit will be the tsx itself.

Thoughts?

@Sahil-Shadwal
Copy link
Contributor Author

We can totally do that. I mean ternary operator could be used, if the markdown is available it will use that variable and if not it will simply lead to tsx itself . Thanks for clearing this out :)

@Sahil-Shadwal
Copy link
Contributor Author

Screencast.from.2024-03-23.15-30-36.webm

Copy link
Collaborator

@benjagm benjagm left a comment

Choose a reason for hiding this comment

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

We are almost there. Left some comments that are all the same case.

Copy link
Member

@DarhkVoyd DarhkVoyd left a comment

Choose a reason for hiding this comment

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

Hey, great work on this. Just a minor suggestion,
benjagm Sahil-Shadwal How about we tweak this a little to do something similar to Jekyll, where they direct the user to create a fork and edit the document with something like:
https://github.com/jekyll/jekyll/edit/master/docs/_docs/datafiles.md
You can try this behaviour on the Jekyll docs here.

@Sahil-Shadwal
Copy link
Contributor Author

Nice suggestion @DarhkVoyd but let's wait for benjagm thoughts!

@benjagm
Copy link
Collaborator

benjagm commented Mar 30, 2024

As of now I think is ok with the current approach. We can implement the fire addition in the future

@Sahil-Shadwal
Copy link
Contributor Author

Yeah sure .

@Sahil-Shadwal Sahil-Shadwal requested a review from benjagm March 30, 2024 19:02
Copy link
Collaborator

@benjagm benjagm left a comment

Choose a reason for hiding this comment

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

We are almost there. For pages with markdown file this work but we need to review the bahaviour for pages without md.

Please see my comment.

@@ -250,7 +255,7 @@ export function DocsHelp() {
target='_blank'
rel='noreferrer'
className='px-[16px] py-[8px] cursor-pointer border-solid border-[#aaaaaa] border rounded-md hover:bg-gray-200 dark:hover:bg-gray-600'
href='https://github.com/orgs/json-schema-org/projects/16'
href={`https://github.com/json-schema-org/website/blob/main/pages${markdownFile ? router.asPath : `/${path}.page.tsx`}.md`}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure this will work for cases where we don't inform markdownFile. As a reminder, if we instance DocsHelp without informing markdownFile, it should build the link with the tsx page but the link generated in this case is wrong.

For example:
http://localhost:3000/overview/sponsors

I am creating the instance of DocsHelp with this code:

<DocsHelp />

Is generating this wrong link:
https://github.com/json-schema-org/website/blob/main/pages/%2Foverview%2Fsponsors.page.tsx.md

But it should be:
https://github.com/json-schema-org/website/blob/main/pages/overview/sponsors/index.page.tsx

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i am working on it .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screencast.from.2024-04-09.15-22-19.webm

Copy link
Collaborator

@benjagm benjagm left a comment

Choose a reason for hiding this comment

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

Your solution works for pages with name "[slug].page.tsx", however that is not working for pages "index.page.tsx" like /overview/sponsors or /overview/code-of-conduct

@Sahil-Shadwal
Copy link
Contributor Author

sorry for all the trouble, i have made changes in markdownFile name for index.page.tsx . Thanks for your patience and guidance :)

Copy link
Collaborator

@benjagm benjagm left a comment

Choose a reason for hiding this comment

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

It worked! Amazing job. Now we are ready to merge!

@benjagm benjagm merged commit 924d1f9 into json-schema-org:main Apr 15, 2024
2 checks passed
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.

Add "Edit this page on GitHub" link to each page
3 participants