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

Natnael.demo practice #269

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

Natnael.demo practice #269

wants to merge 28 commits into from

Conversation

natitedros
Copy link

I have done a basic UI and fetched the data from backend. I am working on posting a new blog and updating it.

@vercel
Copy link

vercel bot commented Aug 2, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
practice ✅ Ready (Inspect) Visit Preview Aug 4, 2022 at 11:02AM (UTC)
practice-project-vue ✅ Ready (Inspect) Visit Preview Aug 4, 2022 at 11:02AM (UTC)
practice-vue ✅ Ready (Inspect) Visit Preview Aug 4, 2022 at 11:02AM (UTC)

AndualemSebsbe and others added 5 commits August 2, 2022 15:54
* [web] Implement Article CRUD
* create the UI

* [web] implement vuex state management

* [web] implement CRUD functionalities

* [web] add authentication and fix previous comments
* [web]: sample structure for the demo

* [web]: folder rename to PascalCase

* [web]: remove feature folder from store

* [web]: fix a bug inside index page

* [web]: fix deployment bug
natitedros and others added 2 commits August 3, 2022 21:31
* [web][yohans] added some basic ui and simple database mocking array

* [web][yohans] added some state for the blogs
* [web]: sample structure for the demo

* [web]: remove feature folder from store

* [web]: file rename issue with git solved

* [WEB]: fix deploy error
commit('newBlog',response.data);
},
async deleteBlog({commit},id){
await axios.delete(`https://blog-app-backend.onrender.com/api/articles/${id}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for an absolute path. Base URL is already set.

Comment on lines 40 to 45
methods:{
toggleAddBlog(){
this.showAddBlog = !this.showAddBlog
},
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Please run npm run lintfix before committing.

Copy link
Contributor

Choose a reason for hiding this comment

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

This will fix indentation and other style problems

Comment on lines 30 to 32
const response = await axios.get(
`https://blog-app-backend.onrender.com/api/articles/${this.$route.params.id}`
)
Copy link
Contributor

Choose a reason for hiding this comment

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

All reading and writing data should be handled by vuex.

Comment on lines 27 to 28
title: '',
content: ''
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead, a better approach is to declare blog object

blog: {
  title: '',
  content: ''
}

Comment on lines 40 to 43
const newBlog = {
title: this.title,
content: this.content
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is unnecessary if the above comment is addressed.

title: this.title,
content: this.content
}
this.addBlog(newBlog)
Copy link
Contributor

Choose a reason for hiding this comment

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

this.addBlog(this.blog)

Copy link
Contributor

@danielgitk danielgitk left a comment

Choose a reason for hiding this comment

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

Do we need to add all the react files to the commit?

Copy link
Contributor

@TripleThreads TripleThreads left a comment

Choose a reason for hiding this comment

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

This PR has changes from another project (React web project). Please make sure your changes are limited to your scope.

@natitedros
Copy link
Author

No. I didn't even know I edited the react project. I think it's by a mistake

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.

8 participants