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

New changes with randomQuestion #237

Closed
wants to merge 5 commits into from

Conversation

Shashank5665
Copy link
Contributor

New feature to add a button to generate random question

@Shashank5665
Copy link
Contributor Author

Hello @seanprashad, as you said, I have created a new draft pull request 😁.
When I added a button in the Header section, it works, but there is also a toggle button and a search bar which is added along with that.
Like the below image :
image

Copy link
Owner

@seanprashad seanprashad left a 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!

@@ -9158,7 +9158,8 @@
"resolved": "https://registry.npmjs.org/fsevents/-/fsevents-2.3.1.tgz",
"integrity": "sha512-YR47Eg4hChJGAB1O3yEAOkGO+rlzutoICGqGo9EZ4lKWokzZRSyIW1QmTzqjtw8MJdj9srP869CuWw/hyzSiBw==",
"os": [
"darwin"
"darwin",
"win32"
Copy link
Owner

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I did! 👍

@@ -362,6 +362,26 @@ const Table = () => {
},
Filter: SelectColumnFilter,
},
// button to display a random question
Copy link
Owner

@seanprashad seanprashad Aug 20, 2022

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:

Screenshot 2022-08-20 at 1 26 55 PM

Copy link
Contributor Author

@Shashank5665 Shashank5665 Aug 20, 2022

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?

Copy link
Owner

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.

Copy link
Contributor Author

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 😊

Copy link
Contributor Author

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.

Copy link
Owner

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!

Copy link
Owner

@seanprashad seanprashad left a 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;
Copy link
Owner

@seanprashad seanprashad Aug 21, 2022

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).

Copy link
Contributor Author

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😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

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 typed these commands till now, I am not sure what to do after this..

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, seems like, after adding these changes..

const questionSlug = questions[questionId].slug; 
window.open(`https://leetcode.com/problems/${questionSlug}/`, '_blank');

There is a bug, the questionSlug is undefined, because of which leetcode is not able to find the page.
image

Copy link
Owner

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  git@github.com:seanprashad/leetcode-patterns.git (fetch)
origin  git@github.com: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 git@github.com: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!

Copy link
Contributor Author

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?

Copy link
Owner

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure @seanprashad !!

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.

2 participants