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

chore: add react router to the project #330

Closed
wants to merge 4 commits into from

Conversation

ftonato
Copy link
Contributor

@ftonato ftonato commented Oct 24, 2021

Description

This pull-request introduces routes into the application, making it possible to share the page we are on with other people easily.

Fixes #294, #329

Type of Change:

  • Code
  • User Interface

Code/Quality Assurance Only

  • New feature (non-breaking change which adds functionality pre-approved by mentors)

How Has This Been Tested?

Checklist:

Delete irrelevant options.

  • My PR follows the style guidelines of this project
  • I have performed a self-review of my own code or materials
  • I have commented my code or provided relevant documentation, particularly in hard-to-understand areas
  • I have attached link of deployed site.

Code/Quality Assurance Only

  • My changes generate no new warnings

/c @isabelcosta, @brittanyjoiner15

@@ -1,5 +1,5 @@
{
"homepage": "./",
"homepage": "https://anitab-org.github.io/",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe there's something that needs to be changed in PR, but we won't know until you merge it.

  • If the project doesn't work, the solution to make it work is to change this line of code by adding this value: https://github.com/anitab-org/anitab-org.github.io/

  • Else, just celebrate 🎉

@vj-codes
Copy link
Member

vj-codes commented Oct 24, 2021

@ftonato hey thank you for the fix!
As per the community guidelines, a contributor has to be first assigned to an issue to create a PR, I was catching up with previous commits and understand it is an effort to make the site better, but please keep the guidelines in mind next time onwards :)
Since you've already created a PR, I'm assigning the issue to you now 🚀

@vj-codes vj-codes added the Status: Needs Review PR needs an additional review or a maintainer's review. label Oct 24, 2021
@@ -14,6 +14,7 @@
"react-native-hyperlink": "0.0.19",
"react-native-reflect": "^0.1.6",
"react-native-web": "^0.12.2",
"react-router-dom": "^5.3.0",
"react-scripts": "^3.4.1"
},
"scripts": {
Copy link
Member

Choose a reason for hiding this comment

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

I am no expert in react but when I was deploying my personal site to GitHub Pages, the scripts part was deploying through a separate gh-pages branch on my repo and not the master branch unlike this one.
The problem I see here is that the master branch is currently 140 commits behind develop, and the recent ones are from testing few PRs, does it lead to any problems @ftonato 👀
Also acc to the documentation it's safer to have a the gh-pages branch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, by default projects tend to separate a branch called gh-pages, but there are those who do it on the master branch, there is no wrong one, and both work equally, it all depends on how people (owners) define it and their strategies.

For example, an application might have production code in the master branch, in the develop branch the code for things they are implementing, and in a docs branch for documentation using gh-pages.

In summary, there are no errors here, here they defined that the master branch is the one that displays the site, so let's go!

Copy link
Contributor

@brittanyjoiner15 brittanyjoiner15 left a comment

Choose a reason for hiding this comment

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

Looks good to me! Sorry this ended up being so much here, but I think the restructure is probably the right way to go about it. I was contemplating doing that when I was in there, but wasn't sure if it made sense to mess things up too much. I guess I still don't entirely understand why this makes more sense for GH pages than what we were doing before? But I am glad it's working!

@ftonato
Copy link
Contributor Author

ftonato commented Oct 25, 2021

Looks good to me! Sorry this ended up being so much here, but I think the restructure is probably the right way to go about it. I was contemplating doing that when I was in there, but wasn't sure if it made sense to mess things up too much. I guess I still don't entirely understand why this makes more sense for GH pages than what we were doing before? But I am glad it's working!

GitHub Pages does not support browser history like your browser does. To overcome this issue, we need to use a Hash router instead of a browser router in our app.

@brittanyjoiner15
Copy link
Contributor

@ftonato ah that makes sense. Hence why it works on local browser but not with gh pages. Thanks for the explanation!

@ftonato
Copy link
Contributor Author

ftonato commented Feb 23, 2023

I'm closing outdated issues and pull-requests that are no longer relevant given how much time has passed since they were opened.

@ftonato ftonato closed this Feb 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Needs Review PR needs an additional review or a maintainer's review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Enable routers on the website
3 participants