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

Remove VisualizerProgramYearObjectives render modifiers #8267

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

michaelchadwick
Copy link
Contributor

Refs ilios/ilios#5374

Two odd things I found:

  1. The link to these visualizations don't show up on demo: https://demo.iliosproject.org/programs/15/programyears/21 (no link), but the same route on local there's a visualization link
  2. The link only works if you click from the previous ProgramYear route: /programs/15/programyears/21 -> click link -> /data/programyears/21/objectives, but not directly. Or if you go there once, and then refresh, you get Zoinks. I'm assuming there's some routing shenanigans that is only working on transition, and not directly.

@michaelchadwick michaelchadwick marked this pull request as ready for review December 13, 2024 17:32
@jrjohnson jrjohnson removed the request for review from stopfstedt December 13, 2024 19:43
const classOfYear = this.intl.t('general.classOf', { year });
this.programYearName = cohort.title ?? classOfYear;

this.data = await this.getData(programYear);
this.data = await this.getData(this.programYear);
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't time out correctly if the program year isn't already loaded. It could return null. This fix is a bit more complicated, but worth the time:

  1. Remove the load entirely
  2. Create a TrackedAsyncData that calls getData using the program year. Until program year exists it should return null.
  3. Probably lots of adjustment and cleanup to get that to work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this one's gonna need more work. Back to the mines...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jrjohnson Any idea why the links to these visualizations only exist on local, but not dev?

Copy link
Member

Choose a reason for hiding this comment

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

@michaelchadwick the visibility of these links is controlled via a feature flag.

for reference, see:
https://github.com/ilios/frontend/blob/master/packages/frontend/app/components/program-year/overview.hbs#L7

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.

3 participants