-
Notifications
You must be signed in to change notification settings - Fork 27
Adds Learners column to Session->Offerings table #8949
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
Adds Learners column to Session->Offerings table #8949
Conversation
✅ Deploy Preview for ilios-frontend ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
I think we need to suppress/remove the truncation from the learner display, and have all names visible. a key issue is for users to have the ability to rapidly verify the assignees without clicking in further. this will take some clever space organization but let's take a stab at it. Alternately, we can modify the mouseover display to be more accessible and legible to users. |
|
I'd vote for the better tooltip. Looks like for learner group hover we're using a popper which we can put HTML in. Going with that instead of a title should make this easier to read. I'm not sure we want to do without truncation on this table or it will be really hard to read everything else. |
bed3128 to
212bb0c
Compare
|
As far as this context goes, showing all Learners but filtered by |
|
SessionsGrid currently shows a Learners column like this PR is attempting to add to Session->Offerings, but it uses the |
|
For Session->Offering context specifically, add individual learners, if they exist, to each existing "Group Name" cell, and filter by |
34b6ebd to
d24c37c
Compare
dartajax
left a comment
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.
@saschaben and I discussed this. We can deal with the counts and labels later. For now, get rid of the count as shown in screen shot if that's cool with you.
abd3008 to
6505a04
Compare
dartajax
left a comment
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.
from my perspective, at least from an initial pass, this looks great - as mentioned we can deal with labels and possibly counts later - good stuff
packages/ilios-common/app/styles/ilios-common/components/session-offerings-list.scss
Outdated
Show resolved
Hide resolved
…p column names for Ls and LGs
…s if the height of the text is too much
…ader; only show learners if exist
…nly learners, only groups, both, or none
…eing done with text or controls
157dea1 to
03c4a8b
Compare
stopfstedt
left a comment
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.
minor issue on an early return statement in a getter. otherwise good.
| } | ||
|
|
||
| get sortedIndividualLearners() { | ||
| if (!this.individualLearners) { |
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.
empty lists are not falsy, and i'm not seeing a falsy value like null or undefined sneaking through here.
| if (!this.individualLearners) { | |
| if (!this.individualLearners.length) { |
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.
Fixed
stopfstedt
left a comment
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.
LGTM





Fixes ilios/ilios#3530