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

Styles #4

Merged
merged 5 commits into from
Aug 15, 2021
Merged

Styles #4

merged 5 commits into from
Aug 15, 2021

Conversation

Meltrust
Copy link
Owner

In this milestone I:

  • Implemented basic required styles.
  • Implemented basic validation.
  • Used localStorage, and implemented DOM event listeners.
  • Used git-flow (main << development << feature).
  • Delivered the project with an organized folder structure.
  • Wrote a descriptive readme file.
  • Used the simplest solutions as possible, over the most complex ones. Readability above all was always the goal.

Thanks for the review! 😄

Copy link

@hemantso hemantso left a comment

Choose a reason for hiding this comment

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

Hi @Meltrust ,

Good job so far! 👍
There are some issues that you still need to work on to go to the next project but you are almost there!

Alt Text

Required Changes ♻️

Check the comments under the review.

Optional suggestions

Every comment with the [OPTIONAL] prefix is not crucial enough to stop the approval of this PR. However, I strongly recommend you to take them into account as they can make your code better.

You can also consider:

  • It will be good if we will add a live link in the README file.

Cheers and happy coding 😃

Feel free to leave any questions or comments in the PR thread if something is not 100% clear. You can mention me (@hemant-soni-vst-au4) so I can see your comments and respond.

Please, do not open a new Pull Request for re-reviews. You should use the same Pull Request submitted for the first review, either valid or invalid unless it is requested otherwise.


As described in the Code reviews limits policy you have a limited number of reviews per project (check the exact number in your Dashboard). If you think that the code review was not fair, you can request a second opinion using this form.

index.html Outdated
Comment on lines 33 to 35
<input type="text" name="Title" placeholder="Title" id="title" />
<input type="text" name="Author" placeholder="Author" id="author" />
<input type="submit" value="submit" class="btn btn-secondary" />
Copy link

@hemantso hemantso Aug 15, 2021

Choose a reason for hiding this comment

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

  • There should be some margin between the input field and submit button as shown in the wireframe. Kindly try to fix it.
    Your
    image

Original
image

Copy link

Choose a reason for hiding this comment

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

This issue is well resolved! ✔️

Copy link

@rloterh rloterh left a comment

Choose a reason for hiding this comment

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

Status: Approved ✔️

Hi Miguel,

Your project is complete! Great job for making the necessary changes recommended by the previous reviewer. There is nothing else to say other than... it's time to merge it 🍾 🍾 :shipit: .
Congratulations! 🎉 🎉 🥂


Cheers and Happy coding!👏 💻 🍷

Feel free to leave any questions or comments in the PR thread if something is not 100% clear.

Please, do not open a new Pull Request for re-reviews. You should use the same Pull Request submitted for the first review, either valid or invalid unless it is requested otherwise.


As described in the Code reviews limits policy you have 1 more limited review per this project. If you think that the code review was not fair, you can request a second opinion using this form.

@Meltrust Meltrust merged commit 2748086 into development Aug 15, 2021
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.

3 participants