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

Fixes#54 Fixes#33 build the root pages & Logout #56

Merged
merged 24 commits into from
Jan 4, 2022

Conversation

aniketkaushik
Copy link
Contributor

Fixes #54
Images are not working.
added condition, only admin can see option for dashboard, Team, Invoice and Report

Fixes #33
Logout Function is added.

@pr-triage pr-triage bot added the PR: draft label Jan 3, 2022
@vipulnsward vipulnsward temporarily deployed to miru-review-pr-56 January 3, 2022 13:39 Inactive
@github-actions
Copy link

github-actions bot commented Jan 3, 2022

Current Code Coverage Percent of this PR:

94.5 %

Files having coverage below 100%

Impacted Files Coverage
/app/controllers/application_controller.rb 42.86 %
/app/controllers/users/registrations_controller.rb 80.0 %

@aniketkaushik aniketkaushik changed the title Fixes#54 build the root pages Fixes#54 Fixes#33 build the root pages Jan 3, 2022
@aniketkaushik aniketkaushik changed the title Fixes#54 Fixes#33 build the root pages Fixes#54 Fixes#33 build the root pages & Logout Jan 3, 2022

<div class="hidden md:mx-3 md:flex md:space-x-2 lg:space-x-8">
<!-- Current: "border-indigo-500 text-gray-900", Default: "border-transparent text-gray-500 hover:border-gray-300 hover:text-gray-700" -->
<% if (current_user.role == "admin" || current_user.role == "owner") %>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<% if (current_user.role == "admin" || current_user.role == "owner") %>
<% if (current_user.admin? || current_user.owner?) %>

Copy link
Contributor Author

@aniketkaushik aniketkaushik Jan 3, 2022

Choose a reason for hiding this comment

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

owner is yet to get defined.

Copy link
Member

Choose a reason for hiding this comment

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

add the owner key to the role enum

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok

@vipulnsward vipulnsward temporarily deployed to miru-review-pr-56 January 3, 2022 13:58 Inactive
<p>Find me in app/views/dashboard/index.html.erb</p>
<div class="" id="root-page">
<%= render "partial/navbar" %>
</div>
Copy link
Member

Choose a reason for hiding this comment

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

add a new empty line

@@ -0,0 +1,5 @@
<% if user_signed_in? %>
Copy link
Member

Choose a reason for hiding this comment

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

remove this condition.
We already have the athenticate_user! condition in the application controller.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok

200: "#BDAEF7",
100: "#DED6FB",
},
'miru-alertgreen': {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'miru-alertgreen': {
'miru-alert-green': {

@aniket-k-kaushik please follow the same pattern in other as well

@vipulnsward vipulnsward temporarily deployed to miru-review-pr-56 January 3, 2022 14:06 Inactive
<div class="hidden md:mx-3 md:flex md:space-x-2 lg:space-x-8">
<!-- Current: "border-indigo-500 text-gray-900", Default: "border-transparent text-gray-500 hover:border-gray-300 hover:text-gray-700" -->
<% if (current_user.admin?) %>
<a href="#" class="border-transparent text-gray-500 hover:text-gray-700 inline-flex items-center px-1 pt-1 text-sm font-medium">
Copy link
Member

Choose a reason for hiding this comment

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

<%= request.path == '/' ? '' : '' %>
You can use this condition for active tab checking.

@vipulnsward vipulnsward temporarily deployed to miru-review-pr-56 January 3, 2022 19:44 Inactive
@vipulnsward vipulnsward temporarily deployed to miru-review-pr-56 January 3, 2022 19:46 Inactive
@vipulnsward vipulnsward temporarily deployed to miru-review-pr-56 January 3, 2022 19:54 Inactive
@vipulnsward vipulnsward temporarily deployed to miru-review-pr-56 January 3, 2022 20:12 Inactive
@vipulnsward vipulnsward temporarily deployed to miru-review-pr-56 January 4, 2022 05:29 Inactive
@akhilgkrishnan akhilgkrishnan force-pushed the 54-build-the-root-pages branch from 859f07d to 69dc7c6 Compare January 4, 2022 05:30
@akhilgkrishnan akhilgkrishnan temporarily deployed to miru-review-pr-56 January 4, 2022 05:30 Inactive
@akhilgkrishnan akhilgkrishnan marked this pull request as ready for review January 4, 2022 05:54
@vipulnsward vipulnsward temporarily deployed to miru-review-pr-56 January 4, 2022 06:04 Inactive
@aniketkaushik
Copy link
Contributor Author

@vipulnsward vipulnsward temporarily deployed to miru-review-pr-56 January 4, 2022 06:31 Inactive
@vipulnsward vipulnsward temporarily deployed to miru-review-pr-56 January 4, 2022 06:34 Inactive
@vipulnsward vipulnsward temporarily deployed to miru-review-pr-56 January 4, 2022 06:45 Inactive
@vipulnsward vipulnsward temporarily deployed to miru-review-pr-56 January 4, 2022 07:02 Inactive
Copy link
Member

@akhilgkrishnan akhilgkrishnan left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@akhilgkrishnan akhilgkrishnan merged commit b8bfa47 into develop Jan 4, 2022
@akhilgkrishnan akhilgkrishnan deleted the 54-build-the-root-pages branch January 4, 2022 07:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build the root page Develop Logout functionality
3 participants