-
-
Notifications
You must be signed in to change notification settings - Fork 484
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
Feat: Added "My Cases" dropdown for volunteers #5272
Feat: Added "My Cases" dropdown for volunteers #5272
Conversation
Fixing the tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Nice work fixing the arrow behavior 👍.
- I saw you added the banners link for admins or supervisor. I believe it would be great to:
- Either Handle this in a separate issue
- Or create an issue and attach that here, it helps in keeping track of the changes.
- Lastly, this is a new feature for volunteers. Please add a couple of test cases to validate this feature.
- Thanks and great work ❤️
app/views/layouts/_sidebar.html.erb
Outdated
</span> | ||
<span class="text"><%= cases_index_title %></span> | ||
</a> | ||
<ul id="ddmenu_cases" class="collapse dropdown-nav"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great to extract this ul
into a partial and use that in both My Cases
and Casa Contacts
@chahmedejaz Oh, sorry about the banner, I messed with my diff, it's related to this pr: #5257, I just removed the changes from this pr! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rest of the changes looks perfect 👍
app/views/layouts/_sidebar.html.erb
Outdated
<%= cases_index_title %> | ||
<% end %> | ||
|
||
<% if policy(:application).see_banner_page? %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Rafael-Martins - Sorry, seems like you mistakenly moved banner changes here 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I did hahaha, Already fixed!
I also createad the partial, made it show dropdown only for volunteer and fixed/added a test to cover the new dropdown!
app/views/layouts/_sidebar.html.erb
Outdated
<% end %> | ||
</li> | ||
|
||
<% if !policy(Volunteer).index? %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Rafael-Martins - That's a good catch. Please leverage the user's role checking methods here i.e.
<% if current_user.volunteer? %>
:
Lines 60 to 70 in 8f08a6a
def casa_admin? | |
is_a?(CasaAdmin) | |
end | |
def supervisor? | |
is_a?(Supervisor) | |
end | |
def volunteer? | |
is_a?(Volunteer) | |
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I was thinking in something like this, thanks!
app/views/layouts/_sidebar.html.erb
Outdated
</li> | ||
<% end %> | ||
|
||
<% if policy(Volunteer).index? %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same can be applied here accordingly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work 👍
There are gonna be merge conflicts here between this and #5086. I can clean them up depending on the order they are merged. |
The links in the sidebar lead to the casa case edit page. Volunteers aren't allowed to edit the case. Could you redirect them to the casa case details page? It's the same as the edit page but with more features for volunteers. |
Changed the link and also the test, just not sure if the failing test in the ci is related to the changes. @FireLemons |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This name could be more specific, since it's not for all dropdowns in the sidebar, but instead for the my_cases dropdown. That being said, don't change this; it will go away with the merge of PR 5086.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! And thanks for fixing the collapsible issue!
What github issue is this PR for, if any?
Resolves #5180
What changed, and why?
My Cases
How will this affect users?
Volunteer: New dropdown in the sidebar
Videos
Dropdown:
casa.dropdown.show.mp4
Old arrow behavior
buttons.old.mp4
Fixed arrow behavior
buttons.fixed.mp4