Skip to content

add modal for reset button #163

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

Merged
merged 2 commits into from
Mar 2, 2022
Merged

add modal for reset button #163

merged 2 commits into from
Mar 2, 2022

Conversation

philiplee13
Copy link
Contributor

Hey @seanprashad - Sorry this took a little bit, still new to the whole react eco system / web development in general.

I have the changes below - basically just a simple modal from "reactstrap" and when the "Reset" button is toggled - it changes the state to show the modal

gh-screen-recording.mov

I originally had tried to make a separate folder under "components" for the modal + logic for it - but I found that moving the "resetHandler" was a bit more tricky than I had planned - so I opted in to just adding the state / modal directly into the "table/index.js"

  • If there was a better way to do this please let me know - I'm open to all suggestions and still learning - thanks for your review on this!

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.

Thank you so very much for this @philiplee13 - it's a change that I'm sure many others will appreciate! 😁

I've tested it locally and everything is good to go - I'd just ask that you trim off the comments you've added. I think the variable/function names are descriptive enough that we can omit them, otherwise we might duplicate meaning. Comments might be useful when we need to dictate why something was done the way it is, for better or worse.

Let me know if you need any help! We're almost across the finish line! 👏🏽

@philiplee13 philiplee13 requested a review from seanprashad March 2, 2022 21:38
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.

A very big thank you for picking this up and following through @philiplee13!

@seanprashad seanprashad merged commit 8b1d380 into seanprashad:master Mar 2, 2022
@philiplee13 philiplee13 deleted the reset-modal branch March 2, 2022 21:41
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