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

Delete Lesson Functionality #89

Open
wants to merge 61 commits into
base: master
Choose a base branch
from
Open

Delete Lesson Functionality #89

wants to merge 61 commits into from

Conversation

Lam7150
Copy link
Contributor

@Lam7150 Lam7150 commented May 18, 2020

Status: πŸš€ Ready

Description 🌟 Allows deletion of lesson with confirmation

Fixes #81

TODOs ⭐

@Lam7150 Lam7150 requested a review from chyku May 18, 2020 18:35
Copy link
Contributor

@chyku chyku left a comment

Choose a reason for hiding this comment

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

some qs and criticisms

@@ -1 +1,44 @@
@import "../../pages/Index/index.scss";

.confirmation-background {
Copy link
Contributor

Choose a reason for hiding this comment

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

alright bc this is frontend i'm assuming @AngelaLuo49021 wrote it correct me if i'm wrong
but to avoid clashing classnames while refactoring i decided to nest all the classes into this one because you can do that in sass, you can look at my branch (i think this is the link: #84 ) for examples

Comment on lines 30 to 33
.yes-no-buttons {
color: black;
background-color: $assignments-light-purple;
font-family: $assignments-roboto-font;
Copy link
Contributor

Choose a reason for hiding this comment

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

pls fix the indentation and rename to .delete-confirmation-button

.confirmation-background {
background-color: white;
border: 4px solid $assignments-font-color;
box-shadow: "5px 10px #888888";
Copy link
Contributor

Choose a reason for hiding this comment

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

are the shadows all similar enough to standardize into a variable?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this one is only used twice

Comment on lines 12 to 13

position: fixed;
Copy link
Contributor

Choose a reason for hiding this comment

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

why the random space

top: 25%;
}

.delete-note{
Copy link
Contributor

Choose a reason for hiding this comment

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

put a space here omg we need a formatter for css

src/components/LessonInfoDisplay/LessonInfoDisplay.js Outdated Show resolved Hide resolved
Comment on lines +60 to +62
function handleDeletedLesson() {
setDeletedLesson(!deletedLesson);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

was this the fix for the lessons not updating problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup! So this signals to the LessonsDisplay to recall the firebase endpoint and update the lessons displayed on the screen in useEffect()

Copy link
Contributor

Choose a reason for hiding this comment

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

could you explain this? is the value of deletedlesson ignored and it's only used to trigger useEffect()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wdym the value of deletedLesson ignored?

Yeah your interpretation is right, I made this specifically just to trigger useEffect(), to signal to the parent component displaying the customLessons (customLessonsDisplay) to update the lessons. Since we're not redirecting back to the same page, I think this is the best way to do it but correct me if I'm wrong if you have a better implementation!

Copy link
Contributor

Choose a reason for hiding this comment

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

i just meant that it doesnt matter if it's true or false because its changing updates the component
i think it should be fine, it's only a little janky

Copy link
Contributor

@chyku chyku left a comment

Choose a reason for hiding this comment

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

almost there bc im a react monster now i found some more stuff for u to do

src/components/LessonInfoDisplay/LessonInfoDisplay.js Outdated Show resolved Hide resolved
$confirmation-light-light-grey-background-color: #EEEEEE;
Copy link
Contributor

Choose a reason for hiding this comment

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

actually can u make this like.. extra-light instead of light light

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.

Delete lesson feature
3 participants