-
Notifications
You must be signed in to change notification settings - Fork 26
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 user context filter to dashboard calendar #7786
base: master
Are you sure you want to change the base?
Conversation
08f3f5e
to
5099ff9
Compare
c140ba9
to
6a33121
Compare
40ff219
to
2450f35
Compare
we'll need this to store user prefs for calendar filters.
this has become obsolete with the inclusion of "learner" as one of the possible user contexts - which makes this control potentially applicable to all users.
this button is dead code. the actual "clear filters" button is somewhere lower on that page.
…test. this requires reworking and fleshing out of existing page objects and tests.
this is more to the point and easier to work with.
d18ec5a
to
6efa1ee
Compare
<ToggleButtons | ||
@firstOptionSelected={{@courseFilters}} | ||
@firstLabel={{t "general.courseOrSessionType"}} | ||
@secondLabel={{t "general.programDetail"}} | ||
@toggle={{@toggleCourseFilters}} | ||
/> | ||
</div> | ||
{{#if this.showClearFilters}} |
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.
Thanks for the commit message, went looking for this and found a super ugly, and very weird button!
@@ -20,14 +21,21 @@ export default class DashboardCalendarComponent extends Component { | |||
|
|||
@tracked usersPrimarySchool; | |||
@tracked absoluteIcsUri; | |||
@tracked user; |
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.
There isn't any point in this.user
being @tracked
as it gets set in the constructor with the setup
method and never changed. Ditch the decorator to avoid future confusion. Or, if you're feeling ambitious, re-write the async setup stuff in the TrackedAsyncData
style.
|
||
return this.ourEvents.filter((event) => { | ||
if ('administrator' === this.userContext) { | ||
// TODO: Replace this with Set.intersect() once that becomes |
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.
Fancy!
/** | ||
* Checks if this user is instructing any offerings or ILMs. | ||
*/ | ||
get isInstructor() { |
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 doesn't work if someone is in an instructor group attached to one of these things. I think the existing allInstructedCourses
property may have what you want already.
setupMirage(hooks); | ||
|
||
test('it renders', async function (assert) { | ||
await render(hbs`<Dashboard::UserContextFilter />`); |
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.
Can you add a a11yAudit
test here?
assert.expect(1); | ||
await setupAuthentication({ school: this.school }, true); | ||
await page.visit({ show: 'calendar' }); | ||
assert.ok(page.calendar.controls.userContextFilter.isPresent); |
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.
Can you add a percySnapshot
here?
assert.expect(2); | ||
await setupAuthentication({ school: this.school }, true); | ||
await page.visit({ show: 'calendar' }); | ||
assert.ok(page.calendar.controls.userContextFilter.isPresent); |
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.
Can you add a percySnapshot
here?
@@ -102,6 +102,7 @@ | |||
"ember-cli-sri": "^2.1.1", | |||
"ember-cli-terser": "^4.0.2", | |||
"ember-load-initializers": "^2.1.2", | |||
"ember-local-storage": "^2.0.7", |
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.
Is this used somewhere? I can't find it.
refs ilios/ilios#5399