-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New changes with randomQuestion #237
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
Conversation
Hello @seanprashad, as you said, I have created a new draft pull request 😁. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is off to a solid start! I've added some comments to help us relocate the code & incorporate your logic into the onClick()
handler! Make the changes and test them out locally on your end.
Also, it looks like your master
branch is outdated - could you please pull the latest version, and then rebase your branch? I think you might have some merge conflicts, but work through them one at a time.
Also, we'll have to update the contents of window.open()
within the onClick()
handler to use slug
instead of url
afterwards!
package-lock.json
Outdated
@@ -9158,7 +9158,8 @@ | |||
"resolved": "https://registry.npmjs.org/fsevents/-/fsevents-2.3.1.tgz", | |||
"integrity": "sha512-YR47Eg4hChJGAB1O3yEAOkGO+rlzutoICGqGo9EZ4lKWokzZRSyIW1QmTzqjtw8MJdj9srP869CuWw/hyzSiBw==", | |||
"os": [ | |||
"darwin" | |||
"darwin", | |||
"win32" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we remove this change from your PR? It looks like you're working on Windows and accidentally added it in!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I did! 👍
src/components/Table/index.js
Outdated
@@ -362,6 +362,26 @@ const Table = () => { | |||
}, | |||
Filter: SelectColumnFilter, | |||
}, | |||
// button to display a random question |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice stuff! We should take this code you've written and move it over to the existing Questions
column instead of creating a new one - may you please make the following changes:
diff --git a/src/components/Table/index.js b/src/components/Table/index.js
index fff57a0..83b30bb 100644
--- a/src/components/Table/index.js
+++ b/src/components/Table/index.js
@@ -14,7 +14,12 @@ import {
import Toggle from 'react-toggle';
import ReactTooltip from 'react-tooltip';
import { useTable, useFilters, useSortBy } from 'react-table';
-import { FaLock, FaExternalLinkAlt, FaQuestionCircle } from 'react-icons/fa';
+import {
+ FaLock,
+ FaExternalLinkAlt,
+ FaRandom,
+ FaQuestionCircle,
+} from 'react-icons/fa';
import {
DefaultColumnFilter,
SelectDifficultyColumnFilter,
@@ -201,7 +206,34 @@ const Table = () => {
},
},
{
- Header: 'Questions',
+ Header: () => {
+ return (
+ <>
+ <div
+ style={{ whiteSpace: 'nowrap', display: 'inline-block' }}
+ >
+ Questions{' '}
+ <Button
+ onClick={() => {
+ const random = Math.floor(
+ Math.random() * questions.length,
+ );
+ const questionId = questions[random].id;
+ const questionSlug = questions[questionId].url;
+ window.open(`${questionSlug}`, '_blank');
+ }}
+ color="dark"
+ id="random-question-button"
+ size="sm"
+ >
+ <span data-tip="Try a random question!">
+ <FaRandom />
+ </span>
+ </Button>
+ </div>
+ </>
+ );
+ },
accessor: 'questions',
disableSortBy: true,
Cell: cellInfo => {
@@ -362,26 +394,6 @@ const Table = () => {
},
Filter: SelectColumnFilter,
},
- // button to display a random question
- {
- randomQuestion: () => {
- const random = Math.floor(Math.random() * questions.length);
- const questionId = questions[random].id;
- return questionId;
- },
- Header: () => {
- return (
- <Button
- onClick={() => {}}
- color="success"
- id="random-question-button"
- >
- Random Question
- </Button>
- );
- },
- accessor: 'randomQuestion',
- },
],
},
],
Here's what it will look like afterwards:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thank you,
Btw, can you please explain to me what the below two lines do, I assume they show the changes, but how do I apply those changes🤔, I am doing it manually by copying and pasting, is that how I do it?
diff --git a/src/components/Table/index.js b/src/components/Table/index.js
index fff57a0..83b30bb 100644
are these two separate commands or just a single command?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah no worries! That line is output from me making the changes locally and then creating a commit so I have it just incase. I could have pushed this commit to your branch, but I'm confident you've got things under control!
As far as applying the changes, you can manually copy and paste them, or we could have done it via git format-patch
: https://devconnected.com/how-to-create-and-apply-git-patch-files/. For example, I would have made the changes on my laptop and then create & uploaded the .patch
for you to apply via git am
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much, I am learning so much with this contribution 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, done with all the changes :D. It works fine, but how do I modify the cellInfo
function such that, it only renders the row whose Id matches with the Question Id generated by the randomQuestionId
function? , I tried with many changes, but it quite didn't work :( as I wanted it to work. Or maybe I shouldn't change the cellInfo
, rather I have to render the matched row from the Header's onClick
function. Basically, I need to create a separate function say generateMatchedRow
which will render the row which is matched with the randomId
and then call it inside the onClick
function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I think there might be a misunderstanding! The code you've written right now actually solves what the original poster wanted in #170:
Would be cool to have a button that randomly selected a problem to study.
This means that we don't need to modify anything else related to cellInfo
- your code is great as is!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're so close! Good work on applying my suggestions!! We just need to rebase and make one small update before we can land this 🙌🏽
const randomQuestion = () => { | ||
const random = Math.floor(Math.random() * questions.length); | ||
const questionId = questions[random].id; | ||
const questionSlug = questions[questionId].url; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were you able to rebase off the master
branch? url
is no longer used there and if we were to land this PR now, it wouldn't work! To rebase means to pull in the latest commits from the branch we're targeting, ie. master
, and then apply all of your commits in the order you made them. It can seem tricky if it's your first time doing it - let me know if you'd like for me to walk you through it 👍🏽
Once you've rebased..
const questionSlug = questions[questionId].url;
will need to be changed to..
const questionSlug = questions[questionId].slug;
and then we'll need to update window.open()
as so:
window.open(`https://leetcode.com/problems/${questionSlug}/`, '_blank');
See questions.json here for how the data is presented - we no longer store the https://leetcode.com/problems/
portion since we know that is "static" in a sense - we only store the unique identifying part of the address, aka a "slug" (MDN docs).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@seanprashad Yes, I am doing this for the first time, so can you please assist me😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I typed these commands till now, I am not sure what to do after this..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PS: I figured it out, I was able to successfully rebase the master branch, and new changes are now present in the master branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you needed to pull from a different remote, ie. the master
branch from seanprashad/leetcode-patterns
instead of master
from Shashank5665/leetcode-patterns
.
We can see the remote repos you have via git remote -v
:
✘ sprashad@home ~/projects/leetcode-patterns randomQuestion git remote -v
origin [email protected]:seanprashad/leetcode-patterns.git (fetch)
origin [email protected]:seanprashad/leetcode-patterns.git (push)
As you can see above, I only have remote for the original repo (ie. the one you're submitting a PR to). You'll need to run the following to add mine as an "upstream" remote:
// This is for SSH - you might use HTTPS
git remote add upstream [email protected]:seanprashad/leetcode-patterns.git
Afterwards, you can verify we've correctly added my repo as a remote via git remote -v
.
I think you can then run the following to pull the latest master
branch from seanprashad/leetcode-patterns
(which has the slug
changes):
git checkout master
git pull upstream master
git checkout -
git rebase master
git push origin -f
Note that we'll need to force push after rebasing - this is because commit hashes change after rebasing, even though the contents of our commit doesn't!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @seanprashad, done with all the steps that you have mentioned, it took me a while to know what's going on, but surfing google a lil bit and with the help of your instructions, I have done it. Now, after starting the server, the feature works very well😁( with the slug inclusion ). I think everything is in place, So, is it just a pull request now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Please do mark it as a PR and I'll review it when I get a chance!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure @seanprashad !!
New feature to add a button to generate random question