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

Create a secret route #279

Closed
wants to merge 2 commits into from
Closed

Create a secret route #279

wants to merge 2 commits into from

Conversation

Haim-S
Copy link

@Haim-S Haim-S commented Dec 5, 2023

I added an isProtected key to the array of paths, which checks which path needs protection.

And then, I built in the route tag, a condition that checks if it's protected, then it doesn't go through, and if so, then it goes through.

Hope you liked it, this is my first open source contribution.

@Haim-S Haim-S requested a review from NoamGaash as a code owner December 5, 2023 09:39
Copy link
Collaborator

@ArkadiK94 ArkadiK94 left a comment

Choose a reason for hiding this comment

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

Hi, thanks for your contribution. I have some comments.

@@ -4,6 +4,7 @@
/node_modules
/.pnp
.pnp.js
*package-lock.json
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should remove this line from here, and use 'yarn' instead of 'npm'.

import React from 'react'
import { Navigate } from 'react-router-dom';

const ProtectedRoute = ({children}:any) => {
Copy link
Collaborator

@ArkadiK94 ArkadiK94 Dec 5, 2023

Choose a reason for hiding this comment

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

Hmm, we don't have authentication on the site so we don't need a Protected Route, the option to get to this page will be through "hidden secret egg" as the storybook.
Please, follow the instructions that were specified in the issue.

@ArkadiK94
Copy link
Collaborator

@NoamGaash can we close this pr?

@Haim-S your contribution is very appreciated even though it wasn't as requested because some misunderstanding. You are welcome to take another issue :)

@NoamGaash
Copy link
Member

@Haim-S @ArkadiK94 it's inactive for a couple of weeks now, I believe it can be closed.
@Haim-S please let us know if you want to reopen it, maybe there's a misunderstanding from our side,
thank you for participating openbus development efforts

@NoamGaash NoamGaash closed this Dec 16, 2023
@Haim-S
Copy link
Author

Haim-S commented Dec 17, 2023 via email

@NoamGaash
Copy link
Member

Hi @Haim-S , thank you!
Feel free to join our discord channel and slack channel.
Also, consider making smaller pull requests - so the development cycle will be shorter. You don't have to solve entire issue in a single pull request - any improvement is a great thing.
how about solving this one?
#80

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