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

not merge this is a proposal #92

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

not merge this is a proposal #92

wants to merge 1 commit into from

Conversation

JuanVqz
Copy link
Contributor

@JuanVqz JuanVqz commented Apr 12, 2019

I think the idea of keeping everything in a view is very good because
it is more user friendly, however it is difficult to do tests,
I think it would be better to design, follow the CRUD convention.

I mention it to continue doing the tests

what do you think?

I think the idea of keeping everything in a view is very good because
it is more user friendly, however it is difficult to do tests,
I think it would be better to design, follow the CRUD convention.

I mention it to continue doing the tests

what do you think?
Copy link
Owner

@changeweb changeweb left a comment

Choose a reason for hiding this comment

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

👏 👏 PR Review 👏 👏
I like your approach. But there are some things that need to be changed a bit.

I won't be able to give time to this project tomorrow since I'll be a bit busy. I'll discuss it here as soon as I can. Be patient and thanks for your time and help that you are contributing so generously 👏 .

@changeweb changeweb added the PR Review 👏 👏 Review Pull Request label Apr 12, 2019
@anishojha
Copy link
Contributor

Yes we have to follow CRUD convention, it will make the code more readable, but all these will do a major change in, so it's the perfect time to do this.
Once we are in stable mode, these type of major changes will be a bit tedious.

@changeweb
Copy link
Owner

I will create a new dev branch to push any new changes before merging on Master branch.

We need to follow 1 step at a time for this big refactoring. What do you think?

@JuanVqz
Copy link
Contributor Author

JuanVqz commented Apr 13, 2019

@changeweb yes it's the perfect way. step at a time, what would you think is the easiest first step?

@JuanVqz JuanVqz closed this Apr 13, 2019
@JuanVqz JuanVqz reopened this Apr 13, 2019
@changeweb
Copy link
Owner

We should do something like this:
Make all changes for a page. Example: Update everything with the change we want. Like:

  1. Edit url in a single page views and Form method to reflect CRUD.
  2. Update the url with the edited url in page views in routes/web.php with new route name e.g.: resource() to reflect CRUD.
  3. Update associated controller methods with the CRUD.
  4. Write Feature test for the page.

@JuanVqz
Copy link
Contributor Author

JuanVqz commented Apr 13, 2019

@changeweb today I have time if you can create a new branch, I'll start like you told.

@changeweb
Copy link
Owner

@JuanVqz ok I'm creating one

@changeweb
Copy link
Owner

@JuanVqz dev_for_new_features branch is created.

@JuanVqz
Copy link
Contributor Author

JuanVqz commented Apr 14, 2019

@changeweb sorry for many problems.

We should set the format of the name of the style attributes, I think it's easier to read with the case of the snake, for example, rowNo to row_no
We should use route than url helper

I think we can follow this guide

or maybe all guide naming conventions

I don't like this format maybe only in classes it's good.

public function foo()
{
  //
}

I'm like this format

public function foo() {
  //
}

but you should set the format...

@changeweb
Copy link
Owner

I try to use snake case for variable name and camel case for function name.
I also prefer 2nd format you liked. But some of the code are in 1st format because they were auto generated.

Some variable names got camel cased because I read an article about snake case vs camel case and wanted to see how it looks when implemented. But I got lazy to update them later. Sorry.

@anishojha
Copy link
Contributor

@JuanVqz @changeweb

Laravel follows the PSR-2 coding standard and the PSR-4 autoloading standard.
Check this link Laravel Coding Style

These are PHP Coding Standard, so we should stick to them

@Soneji
Copy link

Soneji commented Aug 19, 2019

sorry typo. I meant to reference 192 not 92

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR Review 👏 👏 Review Pull Request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants