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

Created the form to search for the courses #326

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Created the form to search for the courses #326

wants to merge 13 commits into from

Conversation

Kevy-loo
Copy link

@Kevy-loo Kevy-loo commented Sep 2, 2022

Description

Created a form where parents can search for available courses when exploring programs for their children.

Co-Authors:

  • Shanae Leslie - ShanaeL29
  • Laila Arkadan - lailaarkadan
  • Kevin Liu - Kevy-loo

Fixes # (issue)

Loom Video

https://www.loom.com/share/c77e0d6d9b674ccab07514954b8235a6

Type of change

  • New feature (non-breaking change which adds functionality)

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have removed unnecessary comments/console logs from my code
  • My changes generate no new warnings
  • I have checked my code and corrected any misspellings
  • No duplicate code left within changed files
  • Size of pull request kept to a minimum

@gclancy121
Copy link
Contributor

Looks good to me, code looks clean

Roadlyfe
Roadlyfe previously approved these changes Sep 6, 2022
Copy link

@Roadlyfe Roadlyfe left a comment

Choose a reason for hiding this comment

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

Code looks good and clean. nice work!

lisamdespain
lisamdespain previously approved these changes Sep 6, 2022
Copy link
Contributor

@lisamdespain lisamdespain left a comment

Choose a reason for hiding this comment

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

Looks really good - you have a lot going on but you implemented it well.

subject,
description,
course_name,
course_description,
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 at some point we need to address that times could be different on different days. Example: Monday class could start at 9am, but Tuesday, class starts at 3pm.

{ title: 'instructor', text: instructor_name },
{ title: 'class size', text: size },
];
// const data = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe ok to delete instead of commenting it out?

@lailaarkadan lailaarkadan dismissed stale reviews from lisamdespain and Roadlyfe via 82685b7 September 9, 2022 04:24
reneetro
reneetro previously approved these changes Sep 9, 2022
Copy link
Contributor

@reneetro reneetro left a comment

Choose a reason for hiding this comment

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

I agree the second set of data that is commented out in the ParentBookingCard.js can probably be removed. You may still want to have a key for each ParentBookCard (perhaps item.id?), that might not be necessary though. Everything else looks nice and organized, it would be awesome to have a Loom so we can see what it looks like!

DawsonReschke
DawsonReschke previously approved these changes Sep 12, 2022
Copy link

@DawsonReschke DawsonReschke left a comment

Choose a reason for hiding this comment

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

Code look great, well written and clean. There was a large object that was commented out, perhaps remove that, otherwise good job!

Copy link

@Austin-T-Johnson Austin-T-Johnson left a comment

Choose a reason for hiding this comment

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

Great job overall, looks like there was a lot implemented on this ticket! I left a few notes in the files changed for your consideration!


const BookingCalendar = () => {
const onPanelChange = (value, mode) => {
console.log(value.format('YYYY-MM-DD'), mode);

Choose a reason for hiding this comment

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

Wondering what this function is intended to do? Looks like currently, it is just console logging the date?

'/children/1/enrollments', // TODO: Hook this request up to pass the ID of the parent/child involved once we have this data in state.
{ child_id: 1, class_id: course_id, completed: true }
)
.post()
.then(res => console.log(res)) // TODO: Let's perform some action with this result.
.catch(err => console.log(`message: ${err.message}`));

Choose a reason for hiding this comment

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

great job adding a catch to handle errors

<Form className="il__top__form" size={'large'} layout="inline">
<Input.Group
compact
// style={{ display: 'flex', flexDirection: 'column' }}

Choose a reason for hiding this comment

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

You might want to remove this commented out code

>
<div>
<div
style={{

Choose a reason for hiding this comment

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

You might want to consider adding the styling in a separate CSS file to clean up the code a bit

Choose a reason for hiding this comment

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

Yes absolutely we were just doing this to prevent switching between screens so much. This is still a draft PR at this point! Thank you!

Copy link

@MaxT6 MaxT6 left a comment

Choose a reason for hiding this comment

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

Nice job creating the form to search for courses. There is a chance to clean up the code but overall the code looks great!

<ParentBookingCard key={idx} booking={item} />
</div>
<div> */}
<ParentBookingCard booking={item} />
Copy link

Choose a reason for hiding this comment

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

Is there a reason this code is commented out?

@@ -161,3 +161,14 @@ div.ant-picker-calendar-date-value {
margin: 0 4%;
}
}
//Calendar Card for
Copy link

Choose a reason for hiding this comment

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

I appreciate the label but what is the calendar card for?

import { dateConverter } from '../../common/dateHelpers';
import { timeConverter } from '../../common/timeHelpers';
import axiosWithAuth from '../../../utils/axiosWithAuth';
import { useOktaAuth } from '@okta/okta-react';
import { addToCart } from '../../../redux/actions/parentActions';
import { dummyData } from '../../../dummyData';
Copy link

Choose a reason for hiding this comment

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

Good job labeling your data as such to indicate it will need to change.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd go one step further here and indicate. 'what kind of dummyData' it is. Example

dummyUserData etc.

}}
>
<button
style={{
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make these styles 'DRY' and not inline. :)

Copy link

@ShanaeL29 ShanaeL29 Sep 23, 2022

Choose a reason for hiding this comment

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

Oh yes we plan to export the styles and make them DRY, we were just using everything inline to prevent switching between screens so much while implementing the figma design. We have actually switched to a dropdown on our current branch. We are implementing ant design styles as well! Thank you for your input Ryan!

Copy link
Contributor

@ryan-hamblin ryan-hamblin left a comment

Choose a reason for hiding this comment

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

I noticed in ParentBookingChild that we're already customizing buttons using inline styles. Let's not do the styles inline, and I would heavily consider the use of AntD's buttons here. Always stick with the easiest path which in this case, is to use the built in library for styling. No need to reinvent button right?

@lailaarkadan lailaarkadan dismissed stale reviews from DawsonReschke and reneetro via 1110bd1 September 27, 2022 02:24
Copy link

@AlexiusThomas AlexiusThomas left a comment

Choose a reason for hiding this comment

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

Great code but I definitely see the areas of improvement.

Copy link

@AlexiusThomas AlexiusThomas left a comment

Choose a reason for hiding this comment

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

i see that testing has started and all checks passed. That is good

@Kevy-loo Kevy-loo marked this pull request as ready for review December 16, 2022 19:29
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.