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

Create add poem feature #73

Merged
merged 7 commits into from Aug 19, 2019

Conversation

@Dalmano
Copy link
Contributor

commented Aug 14, 2019

Relates #33

Dalmano added 3 commits Aug 15, 2019
const [title, setTitle] = React.useState("");
const [poem, setPoem] = React.useState("");
const [error, setError] = React.useState(false);
const [typeError, setTypeError] = React.useState(false);

This comment has been minimized.

Copy link
@engshorouq

engshorouq Aug 17, 2019

Contributor

I think there is no type in the poem

const [typeError, setTypeError] = React.useState(false);
const [fetchError, setFetchError] = React.useState(false);
const [buttonContent, setButtonContent] = React.useState("Save");
const acceptedTypes = "";

This comment has been minimized.

Copy link
@engshorouq

engshorouq Aug 17, 2019

Contributor

type for what? 🤔

fetch: "Sorry something went wrong, Please try again"
};

let type = poem.name.split(".")[poem.name.split(".").length - 1];

This comment has been minimized.

Copy link
@engshorouq

engshorouq Aug 17, 2019

Contributor

I think for the poem just has title and content


setButtonContent("Loading...");

const poemData = new poemData();

This comment has been minimized.

Copy link
@engshorouq

engshorouq Aug 17, 2019

Contributor

there is no class name is poemData Do you mean FormData ?? 🤔

<label htmlFor="new-poem__text">Add a poem</label>
<input
id="mew-poem__text"
type="text"

This comment has been minimized.

Copy link
@engshorouq

engshorouq Aug 17, 2019

Contributor

I think best if we put the type textarea

<input
id="mew-poem__text"
type="text"
onChange={e => setPoem(e.target.poems[0])}

This comment has been minimized.

Copy link
@engshorouq

engshorouq Aug 17, 2019

Contributor

mmmm there is poems inside the target 🙄

<a href="/poems" className="large-skip__button">
Back
</a>
<button className="large-save__button" type="submit">

This comment has been minimized.

Copy link
@engshorouq

engshorouq Aug 17, 2019

Contributor

I think there is Button Component why don't use it and where the onClick for this button

)}
{fetchError && <p className="add-poem__error">{message.fetch}</p>}
<div className="add-poem__form__button">
<a href="/poems" className="large-skip__button">

This comment has been minimized.

Copy link
@engshorouq

engshorouq Aug 17, 2019

Contributor

why use a tag also there is Button component and use props.history.goBack()

const message = {
default: "Please add a title and add a poem",
tyrebased:
"Please upload a different poem, we do not support that poem type",

This comment has been minimized.

Copy link
@engshorouq

engshorouq Aug 17, 2019

Contributor

I think there is no uploading in this file. 🤔

@dubhcait
Copy link
Collaborator

left a comment

You never pushed your last commit, I remember you changed it a lot. Easily done.

const [title, setTitle] = React.useState("");
const [poem, setPoem] = React.useState("");
const [error, setError] = React.useState(false);
const [typeError, setTypeError] = React.useState(false);

This comment has been minimized.

Copy link
@IsraaSulaiman

IsraaSulaiman Aug 19, 2019

Contributor

This line is not necessary

Suggested change
const [typeError, setTypeError] = React.useState(false);
e.preventDefault();

if ((title === "") | (poem === "")) {
console.log(title, poem);

This comment has been minimized.

Copy link
@IsraaSulaiman

IsraaSulaiman Aug 19, 2019

Contributor
Suggested change
console.log(title, poem);

setButtonContent("Loading...");

fetch("/api/v1/send-poem", {

This comment has been minimized.

Copy link
@IsraaSulaiman

IsraaSulaiman Aug 19, 2019

Contributor

can you change the api url to /api/v1/poems? It is a best practise to use restful apis.
Here is a link if you're interested:
https://restfulapi.net/resource-naming/

}}
/>

<Input

This comment has been minimized.

Copy link
@IsraaSulaiman

IsraaSulaiman Aug 19, 2019

Contributor

As shorouq said, it is better to have textarea instead of input?

@Dalmano Dalmano merged commit e297082 into staging Aug 19, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@IsraaSulaiman IsraaSulaiman deleted the feature/add-poem branch Aug 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.