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

Sponsor Form Added #118

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

Conversation

ayush3160
Copy link
Contributor

@ayush3160 ayush3160 commented Feb 16, 2023

fix #102 , I have implemented the sponsor details form. The revised flow of creating the event is as follow, At the time of creating a new event only the basic-detail form is accessible and after submitting the form , it will redirect to the edit event page of sponsor from where the user can add and edit the sponsors , speaker's etc.

Rocket.Chat_.Communications.Platform.You.Can.Fully.Trust.-.17.February.2023.mp4

@ayush3160
Copy link
Contributor Author

@Dnouv , Can you please review this PR and please let me know if any changes are required.

Copy link
Member

@Dnouv Dnouv left a comment

Choose a reason for hiding this comment

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

Hey @ayush3160

Thanks for the PR. I have left some comments; please have a look, and let me know if you have any questions. Thank you!

Comment on lines +161 to +170
export const getSponsorsDetails = async (eid,auth) => {
const headers = {
Accept: "application/vnd.api+json",
Authorization: `JWT ${auth}`,
};
const res = await axios.get(`${eventUrl}/v1/events/${eid}/sponsors`, {
headers: headers,
});
return res;
}
Copy link
Member

Choose a reason for hiding this comment

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

We are using the same call on the EventDisplay page, and this would result in an error. Since the Event Display page does not requires the user to be logged in.

Also, if possible please reduce introducing new axios calls, we are trying to move all the API calls, to native fetch

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 have added a new event details call similar to getEventDeatils but with the auth because to fetch the details of draft event we will need the auth.

Comment on lines 166 to 168
<Button variant="primary" type="submit" onClick={handleSubmit} className="my-2">
Next
</Button>
Copy link
Member

Choose a reason for hiding this comment

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

If the user clicks on the "Next" button on the Basic detail form, the event is supposed to be saved as draft, so we need an "Publish" button (if the event is in draft state)

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 have added the Publish Event Button if the event is in the draft state

<Form.Control
type="number"
onChange={e => {props.handleChange(e,id)}}
name="level"
Copy link
Member

Choose a reason for hiding this comment

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

We should not allow negative levels. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was no such clarification in the Api documentation but I also think that the levels should be 0 or more than 0, So I have added that condition in the form.

</div>
);
})
: "No Sponsor found"}
Copy link
Member

Choose a reason for hiding this comment

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

This text should be something like greyed out or italic. To differentiate with other texts.

Or I would highly suggest having a toggle switch with title "Enable Sponsors," which by default will be toggled off, but when toggled on, it should show the question forms embedded inside the component, not as modal.

Thank you!

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 have changed the text in italic and also changed the color to grey.

@Dnouv
Copy link
Member

Dnouv commented Feb 27, 2023

Hey @ayush3160

Any updates? If there are, please let me know. Thank you!

@ayush3160
Copy link
Contributor Author

@Dnouv , I am really sorry for delaying this. Please have a look on the changes that I have made.
Also I wanted to ask that is there any reason for going towards the embedded questions form because we are using the modal's in speaker and sessions. Also I am changing the isSponsorEnabled to true when the user is adding the first sponsor or if it is disabled. So, I don't think we need a toggle for that. Just let me know about this and I'll make the change's accordingly.

Thanks You.

Comment on lines +146 to +148
const res = await fetch(`${eventUrl}/v1/events/${eid}?include=tickets`, {
headers: headers
});

Check failure

Code scanning / CodeQL

Server-side request forgery

The [URL](1) of this request depends on a [user-provided value](2).
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.

[TODO] Create a new form component to fill in the Sponsor Details
2 participants