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

User Mentee Applications: add ability for admins to view all notes associated with status #450

Conversation

jp524
Copy link
Contributor

@jp524 jp524 commented Dec 4, 2023

What's the change?

On the admin side of User Mentee Application, all reviewer notes are shown in one place using a select tag.
The associated Stimulus controller displays a note based on the status selected.

What key workflows are impacted?

Updating an applicant's status.

Highlights / Surprises / Risks / Cleanup

When updating an applicant status from the admin_user_mentee_application_path(:id), the select element is not automatically updated. Therefore a manual refresh of the page is required to display the new status and note.
I thought about adding a Turbo action to update this select tag but since I am using ViewComponents I don't think that is possible.

Demo / Screenshots

Screen.Recording.2023-12-04.at.15.51.09.mov

Issue ticket number and link

Closes #429

Checklist before requesting a review

  • Did you add appropriate automated tests?
  • Is everything elevated to the highest level? (e.g., can logic be moved out of view into controllers, or out of controllers into models?)
  • Did you add any relevant tags and decide if this PR is a Draft vs. Ready for Review?

Copy link

codecov bot commented Dec 4, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a8617c2) 98.94% compared to head (7bb77f2) 98.95%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #450   +/-   ##
=======================================
  Coverage   98.94%   98.95%           
=======================================
  Files         215      217    +2     
  Lines        3223     3246   +23     
=======================================
+ Hits         3189     3212   +23     
  Misses         34       34           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JoshDevHub
Copy link
Collaborator

This is awesome! Couple comments:

  1. I thiiink having this work with turbo would be as simple as re-rendering the component in the existing create turbo stream. Here's the existing code to update the note section. Instead of:

      <%= @user_mentee_application.current_state.note %>

    You can render the component there. And then where you're rendering the component on the show page, make sure the span has an id like "#{dom_id(@user_mentee_application)}--note" -- this way when a new status is created, it'll know to rerender your view component in that note section. Let me know if this doesn't work or you have any questions about it.

  2. This is a bit more of a theoretical design thing. You've modeled this as a select input, but I think you could also model it as a dropdown, where each "item" is a link to say, NotesController#show (which you'd have to make). And this is wired up using turbo frames such that when the admin clicks a link in the dropdown, the notes frame would get replaced with the content for the selected item. Hopefully that makes sense 😅
    Anyway, I'm not saying for sure this would be a "better" way of doing it, and I'd be interested in knowing others' thoughts. But I also know a lot of people view using select inputs outside of a form context for this type of usecase as a codesmell. Would be interested in @afogel and @toyhammered thoughts on this.

@afogel
Copy link
Collaborator

afogel commented Dec 4, 2023

@jp524 Looks really great! I think I agree w/ @JoshDevHub's assessment on using the select; it does strike me as though the affordance of that may not be as ideal, both from a UI design standpoint as well as a UX/code perspective. Like Josh mentioned, I typically think of selects in terms of forms. That having been said, I think the way you're using them isn't out of bounds.

One thing I liked about your choice to use the select element over the accordion is that it provides a much more streamlined/space efficient experience compared to the accordion, especially as we get further in the process. My initial instinct is that a tabbed component might be a better experience, but it still suffers from some of the issues of space efficiency and finding what's relevant as the process advances onward.

As you mentioned, it isn't ideal to have to refresh the page to see the new changes reflected in the page. One alternative interface might be to do something like this:
Screenshot 2023-12-04 at 9 26 09 PM

This is kind of like a vertical tab situation and I think would dovetail well with what Josh suggested -- when you create the note and advance the applicant, you can use a turbo frame to prepend the latest status to the box on the left hand side and replace the main content area with a turbo frame containing notes on that page. In this conceptualization, each link on the left hand side representing a previously completed stage in the application process that would fetch the associated notes and replace the main content area.

That said, if you can get it working with turbo and the select, this might be an instance where perfect is the enemy of good enough. What do you think?

@jp524 jp524 requested review from afogel and removed request for toyhammered December 18, 2023 19:51
@jp524
Copy link
Contributor Author

jp524 commented Dec 18, 2023

Thank you @JoshDevHub and @afogel for your insights! I liked seeing your approaches to the problem and I can see now that it can definitely be solved without JavaScript.

I ended up keeping the select tag solution for now, and simply re-renderedthe NotesComponent through a Turbo Stream.

@jp524 jp524 marked this pull request as ready for review December 18, 2023 19:54
Copy link
Collaborator

@JoshDevHub JoshDevHub left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@JoshDevHub JoshDevHub merged commit 6799b68 into main Dec 20, 2023
3 checks passed
@JoshDevHub JoshDevHub deleted the 429-usermenteeapplication-20admin-ability-to-view-the-notes-of-each-stage-for-an-applicant branch December 20, 2023 01:00
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.

[UserMenteeApplication-2.0][Admin] Ability to view the notes of each stage for an applicant
3 participants