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

Edit page #4310

Closed
wants to merge 12 commits into from
Closed

Edit page #4310

wants to merge 12 commits into from

Conversation

8rxn
Copy link
Member

@8rxn 8rxn commented May 29, 2023

Description

This PR fixes #4297

Notes for Reviewers

Signed commits

  • Yes, I signed my commits.

@l5io
Copy link
Contributor

l5io commented May 29, 2023

🚀 Preview for commit e3b0495 at: https://6474de915889952dcf239c1f--layer5.netlify.app

@leecalcote
Copy link
Member

@8rxn hint: you should only need to be concerned linking to the respective page here: https://github.com/layer5io/layer5/tree/master/src/pages. Beyond this, we won't be able to navigate contributors to the specific component. In some cases, they'll have to navigate to more deeply beyond the page source link. // @Shivam-AfA

@8rxn
Copy link
Member Author

8rxn commented Jun 1, 2023

@8rxn hint: you should only need to be concerned linking to the respective page here: https://github.com/layer5io/layer5/tree/master/src/pages. Beyond this, we won't be able to navigate contributors to the specific component. In some cases, they'll have to navigate to more deeply beyond the page source link. // @Shivam-AfA

Thanks @leecalcote for the insight

Indeed that's what I am implementing.
Created a template string than resulted in the file url on github depending upon the location prop.

@github-actions github-actions bot added area/blog New posts or new blog functionality area/careers area/community area/projects An issue relating to Layer5 initiatives (projects) labels Jun 1, 2023
@8rxn 8rxn marked this pull request as ready for review June 1, 2023 16:26
DarrenDsouza7273 and others added 5 commits June 1, 2023 22:01
Signed-off-by: Darren Dsouza <darrendsouza7273@gmail.com>
Signed-off-by: Darren Dsouza <darrendsouza7273@gmail.com>

Add edit link

Signed-off-by: 8rxn <rajxryn@gmail.com>

demo placement of Edit button

Signed-off-by: 8rxn <rajxryn@gmail.com>

tweak editlink

Signed-off-by: 8rxn <rajxryn@gmail.com>
Signed-off-by: 8rxn <rajxryn@gmail.com>
Signed-off-by: 8rxn <rajxryn@gmail.com>
Signed-off-by: 8rxn <rajxryn@gmail.com>
Signed-off-by: Raj Aryan <75237697+8rxn@users.noreply.github.com>
@8rxn
Copy link
Member Author

8rxn commented Jun 1, 2023

Done with the implementation. Open to Reviews.
image

@8rxn 8rxn requested a review from Shivam-AfA June 1, 2023 16:41
8rxn and others added 5 commits June 1, 2023 22:26
@8rxn
Copy link
Member Author

8rxn commented Jun 1, 2023

Made a few more commits:

Deleted files accidentally pushed. Fixed the pathname, should not be failing anymore, missed footer-landscape-page for footer, added that.

Edit : Noticed it still failed, I'll be reworking on it.

Signed-off-by: 8rxn <rajxryn@gmail.com>
@l5io
Copy link
Contributor

l5io commented Jun 1, 2023

🚀 Preview for commit d16d1d6 at: https://647911dd932d58184e48d0f4--layer5.netlify.app

@8rxn
Copy link
Member Author

8rxn commented Jun 1, 2023

Implemented fixes pointed by @randychilau on slack.
Link works 🚀

@Shivam-AfA
Copy link
Contributor

Shivam-AfA commented Jun 2, 2023

@8rxn Thanks for this! Let's showcase your efforts and ask for feedback on Monday's websites call before getting it live.

@8rxn
Copy link
Member Author

8rxn commented Jun 2, 2023

@8rxn Thanks for this! Let's showcase your efforts and ask for feedback on Monday's websites call before getting it live.

Sure @Shivam-AfA
I do believe that there could maybe be some polish needed.
I changed a large number of files so it was kind of hard to track whether or not everything works flawlessly or not.
However I did test a few different type of pages and this seemed perfectly but I could have missed something or the other.

Copy link
Contributor

@Nikhil-Ladha Nikhil-Ladha left a comment

Choose a reason for hiding this comment

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

Few issues needed to be taken care of:
The hover effect on the link is incorrect. The cursor changes to pointer on the Get Involved heading as well, which should not be the case. Also, restrict the width to fit-content such that the whole row (upto the column width) is not hoerable and only the text is.

Since, you have removed the Footer component from the Layout component all the collection pages like blogs, resources etc doesn't have the Footer now (eg: https://647911dd932d58184e48d0f4--layer5.netlify.app/blog/community/building-amazing-open-source-tools-one-contributor-at-a-time). That has to be fixed. You could handle these cases by pointing them to individual pages. Also, keep in mind that the content for learning-paths is stored in a separate directory named content-learn so a special case for that needs to be handled as well.
Secondly, what if we pass the location property to the Footer component directly from the Layout compoonent, does it not get the actual page location in its porps?

@8rxn
Copy link
Member Author

8rxn commented Jun 3, 2023

Thanks for the insights @Nikhil-Ladha
I'll be taking care of these issues.
Regarding the Layout, the location prop is only accessible from inside the pages, hence it does not work that way. I did try that beforehand just to avoid changing a large number of files.

@8rxn
Copy link
Member Author

8rxn commented Jun 4, 2023

With great help from @randychilau , I have worked on the issue in another branch with a better implementation and created a new PR for the same.
The changes requested in this PR have been implemented on that branch. Link to new PR : #4334
Requesting reviews from @Nikhil-Ladha
I'll be closing this PR.

@8rxn 8rxn closed this Jun 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/blog New posts or new blog functionality area/careers area/community area/projects An issue relating to Layer5 initiatives (projects)
Development

Successfully merging this pull request may close these issues.

Add an 'Edit this page' link on layer5.io
6 participants