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

Added create event button, page and form #162

Merged
merged 2 commits into from
Sep 15, 2020
Merged

Conversation

thai-truong
Copy link
Collaborator

Also added a QueriedEventPage to handle conditional rendering via query params on /events endpoint.

@thai-truong thai-truong added the feature Build in something new label Sep 15, 2020
@thai-truong thai-truong requested a review from a team September 15, 2020 03:49
@thai-truong thai-truong self-assigned this Sep 15, 2020
@netlify
Copy link

netlify bot commented Sep 15, 2020

Deploy preview for hknucsd-portal-dev ready!

Built with commit 0f2d063

https://deploy-preview-162--hknucsd-portal-dev.netlify.app

Copy link
Collaborator

@godwinpang godwinpang left a comment

Choose a reason for hiding this comment

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

couple things - let's merge in, make some issues and clean stuff up after demo

@@ -4,7 +4,7 @@ import { BaseAutocomplete } from '../base';

import { getMultipleUsers, getUserNames } from '@Services/UserService';

type OfficerNameData = {
export type OfficerNameData = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

should later standardize type vs interface for props - nbd though

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll probably be switching to using interface instead of type, since interface is more common in our codebase.

@@ -46,3 +46,7 @@ export const OfficerNameAutocomplete = (props: OfficerAutocompleteProp) => {
/>
);
};

OfficerNameAutocomplete.defaultProps = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

can drop defaultProps with line 20

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Weird enough, eslint gave me an error for not having defaultProps even tho I already set a default value lmao...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You know any way of fixing that? Idk why it's happening lol. Worst case scenario I might have to turn that off by line

@@ -23,4 +23,8 @@ const EventStatusDropdownField = (props: EventStatusFieldProp) => {
);
};

EventStatusDropdownField.defaultProps = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

again can drop defaultProps given line 13

@@ -23,4 +23,8 @@ const EventTypeDropdownField = (props: EventTypeFieldProp) => {
);
};

EventTypeDropdownField.defaultProps = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

see above

handleSubmit(values, setSubmitting);
}}
>
{({ submitForm, isSubmitting }) => (
Copy link
Collaborator

Choose a reason for hiding this comment

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

later extract out form into component to be reused - we should aim to extract out components sooner than later

* This is for conditional rendering of component(s) at /events endpoint based on
* the endpoint's query parameters
*/
function QueriedEventPage(): JSX.Element {
Copy link
Collaborator

Choose a reason for hiding this comment

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

we shouldn't have a separate page for this - we should be switching on params in '/events' instead

Copy link
Collaborator Author

@thai-truong thai-truong Sep 15, 2020

Choose a reason for hiding this comment

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

What do you mean switching on params in '/events'? You mean do it at the App level?? Route's path prop doesn't recognize query params

Copy link
Collaborator

Choose a reason for hiding this comment

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

page level, /events and /events?x=true is the same page

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah but we don't have a page on /events yet, the Get Involved/Events page is on /

Copy link
Collaborator

Choose a reason for hiding this comment

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

wait what's the calendar under? I think it would make sense to put it as events

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's currently under /calendar

@thai-truong thai-truong merged commit 954fb42 into dev Sep 15, 2020
godwinpang pushed a commit that referenced this pull request Sep 20, 2020
* Tsconfig changes again : (

* Added create event button, page and form.
@godwinpang godwinpang deleted the create_event_btn_form branch September 20, 2020 22:09
thai-truong added a commit that referenced this pull request Oct 3, 2020
* Tsconfig changes again : (

* Added create event button, page and form.

Rebase modal_refactor's commits onto master branch (in progress)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Build in something new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants