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

add add policy function and ordering the home & layout #41

Merged
merged 5 commits into from
May 10, 2020

Conversation

naamaCohen1
Copy link
Collaborator

I added the add policy function and ordering the layout & home page that all pages will be in footer.
I also change the version of the jQuaey because the post function didn't succeeded in jQuery slim version.

edison/static/js/policy.model.js Outdated Show resolved Hide resolved
edison/static/js/policy.model.js Outdated Show resolved Hide resolved
edison/templates/layout.html Show resolved Hide resolved
edison/templates/layout.html Outdated Show resolved Hide resolved
edison/templates/policy.html Outdated Show resolved Hide resolved
edison/templates/policy.html Outdated Show resolved Hide resolved
edison/static/js/policy.controller.js Outdated Show resolved Hide resolved
edison/static/js/policy.controller.js Outdated Show resolved Hide resolved
edison/static/js/policy.controller.js Show resolved Hide resolved
edison/static/js/policy.controller.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@DoRTaL94 DoRTaL94 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@RoniRush RoniRush left a comment

Choose a reason for hiding this comment

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

Few notes for next time -

  1. I personally recommend to add more comments in the code OR give more significants names to variables and functions ( for example a variable called "ac" for "air-conditioner", for other people and even for you in the future, it might be a bit complicated to understand what's this variable stands for)

  2. I think this PR is a little bit big, I'm sure there was a way to split it , even 2 PR's instead of one

over-all I think you did an AWSOME job !
LGTM :)

@naamaCohen1
Copy link
Collaborator Author

OK, thanks! I will be on this now ,and yes the PR was a big one, I now know for the future to split the PR's

Copy link
Collaborator

@RoniRush RoniRush left a comment

Choose a reason for hiding this comment

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

LGTM :)

@naamaCohen1 naamaCohen1 merged commit d7b20f8 into redhat-beyond:master May 10, 2020
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