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

Fix display of report item links in Compare View. #407

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

Conversation

lublagg
Copy link
Contributor

@lublagg lublagg commented Oct 24, 2022

No description provided.

@codecov
Copy link

codecov bot commented Oct 24, 2022

Codecov Report

Base: 94.28% // Head: 94.28% // No change to project coverage 👍

Coverage data is based on head (a5707ac) compared to base (a0840c5).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #407   +/-   ##
=======================================
  Coverage   94.28%   94.28%           
=======================================
  Files         163      163           
  Lines        5813     5813           
  Branches     1175     1175           
=======================================
  Hits         5481     5481           
  Misses        307      307           
  Partials       25       25           
Flag Coverage Δ
cypress 92.47% <100.00%> (ø)
jest 63.07% <80.00%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
js/components/report/compare-view.js 76.92% <ø> (ø)
js/components/report/answer.js 96.00% <100.00%> (ø)
js/components/report/iframe-answer.tsx 96.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@cypress
Copy link

cypress bot commented Oct 24, 2022



Test summary

175 0 5 0


Run details

Project portal-report
Status Passed
Commit a5707ac
Started Oct 24, 2022 7:29 PM
Ended Oct 24, 2022 7:33 PM
Duration 04:11 💡
OS Linux Ubuntu - 20.04
Browser Electron 91

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@lublagg lublagg changed the title WIP: Fix display of report item links in Compare View. Fix display of report item links in Compare View. Oct 24, 2022
Copy link
Member

@dougmartin dougmartin left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

I think you should wait for @scytacki to also review since we paired to do this work.

Copy link
Member

@scytacki scytacki left a comment

Choose a reason for hiding this comment

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

I think this will suffer all of the same problems as the original PR.
https://github.com/concord-consortium/portal-report/pull/404/files

It just makes it more explicit that that this flag is for the compare view. But I think there are several interactives that should be showing their report items in the compare view.

@@ -213,7 +214,7 @@ export class IframeAnswer extends PureComponent<IProps, IState> {
{maybeAnswerTextOrLinks}
</div>
)}
{reportItemAnswerItems.map((item, index) => (
{!compareView && reportItemAnswerItems.map((item, index) => (
Copy link
Member

Choose a reason for hiding this comment

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

Have you checked what happens with open response questions in this case?
As I wrote in the other PR. I think this will prevent open response questions from showing all of there features. It will also prevent something like SageModeler from showing its metrics in the compare view.

@scytacki
Copy link
Member

@lublagg and @dougmartin I think the best incremental work to be done on this is to document the cases. Perhaps I'm wrong, but if I'm right then have this documentation will make it easier for devs and owners to understand how this isn't a simple problem.

So making screenshots of what happens with and without this PR for Open Response, Multiple Choice, Sage Modeler, and the bar graph.

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