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

Misc Perseus renderer style fixes #8714

Conversation

nucleogenesis
Copy link
Member

@nucleogenesis nucleogenesis commented Nov 16, 2021

Summary

  • Fix error indicated by KFixedGridItem re: too-big span (the red error rectangle)
  • CSS was left behind when the LearningActivityBar was moved to AssessmentWrapper - bringing it over fixes the hint tooltip going off screen and aligns the tooltip
  • Fix letter-spacing being set to some silly number by Perseus, resulting in smushed letters
  • #solutionarea is 100% width now to avoid horizontal overflow (it would be larger for no apparent reason)
  • Some unnecessary vertical padding removed from .problem-area

BEFORE

before.1.mp4

AFTER

after.mp4

References

No related issues (that I know of or have seen) - but spurred by notes in Slack from @radinamatic

Reviewer guidance

How does it look? Are there any kinds of exercise that I should review specifically? Some of these issues only affect those which have a grid of options and I saw no other issues with the fill-in-the-blank or drag and drop ones I have locally.


Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

- Fix error indicated by KFixedGridItem re: too-big span (the red error rectangle)
- CSS was left behind when the LearningActivityBar was moved to AssessmentWrapper - bringing it over fixes the hint tooltip going off screen and aligns the tooltip
- Fix `letter-spacing` being set to some silly number by Perseus, resulting in smushed letters
- #solutionarea is 100% width now to avoid horizontal overflow (it would be larger for no apparent reason)
- Some unnecessary vertical padding removed from .problem-area
@nucleogenesis nucleogenesis added the TODO: needs review Waiting for review label Nov 16, 2021
@nucleogenesis nucleogenesis added this to the 0.15.0 milestone Nov 16, 2021
Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

Nothing blocking - just some questions!

@@ -615,9 +617,10 @@
padding-right: 16px;
}

.hint-btn-container {
/deep/ .hint-btn-container {
Copy link
Member

Choose a reason for hiding this comment

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

Is this class still relevant any more? We've moved it out of the perseus renderer, so this might just be left over from my failing to clean it up?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yep I copied this over but ought to have cut, will remove shortly

@@ -15,7 +15,7 @@
/>
</div>
<KGrid>
<KGridItem :layout="{ span: 8 }">
<KGridItem :layout="{ span: 4 }">
Copy link
Member

Choose a reason for hiding this comment

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

This does seem to produce quite a narrow display on a fairly wide screen from your animation - maybe we should conditionalize these values for different aspect ratios?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've revised the layouts now to go :layout12={ span: 8 } to get a ~66% width on larger screens. Then the default to 100% on medium or smaller which seems to avoid having too-squished on tablet & mobile without having it too wide on a larger screens.

The below video goes from ~1100px wide down to like ~200px wide at smallest. I then show it with the hints enabled because the layout affects it.

Previously the :layout="{ span: 4 }" would be 4/12 at large, 4/8 at tablet and 4/4 at mobile - so when it was set to :layout="{span: 8}" it complained about the span exceeding the num cols available (4 at mobile size).

better-layout.mp4

@radinamatic
Copy link
Member

Latest APK asset installed on Motorola G6 (Android 9):

Error Fix Comment Screenshot
the red error rectangle ✔️ ... ...
hint tooltip going off screen and aligns the tooltip Tooltip itself is not responsive DevTools - localhost:8080-en-learn---home_002
smushed letters ✔️ ... ...
avoid horizontal overflow ⁉️ Still some overflow in other resources, not sure if it's supposed to remain in some cases overflow2
vertical padding removed ⁉️ I find that the padding around the problem area still wastes a lot of space... If margins for the rest are (around) 40px, why does the actual assessment have this huge white space around? DevTools - localhost:8080-en-learn---home_003

@radinamatic
Copy link
Member

I'm also still seeing the title of the exercise being cut out and not using all the available space in the row:

1 2
perseus2 perseus1

@nucleogenesis
Copy link
Member Author

@radinamatic

  1. Improved Padding on Mobile
    image

  2. Title gets squished and stays that way - this is out of scope here and is more of a bug with TextTruncator that it isn't responsive. If it gets truncated too far it's always going to be that truncated until page refresh if you widen your viewport.

  3. Horizontal overflow - I cannot replicate this with a similar exercise in the QA channel. I suspect this is going to be unique to certain questions. Hopefully the padding reduction mitigates the issue as well.

Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

I don't think exercises have ever looked so good! As long as @radinamatic greenlights this, we're good to go!

@radinamatic
Copy link
Member

Ooohhh, can't wait to test this on my phone... 😍
Thank you so much for making a better use of the available space!

@MisRob already took care of the squished title in #8678, so as @rtibbles said, exercises are looking goooooood!!! 🙂

Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

Going to approve this to get it in, if any more issues do pop up in testing, let's open follow ups.

@rtibbles rtibbles merged commit 9b9d7a3 into learningequality:release-v0.15.x Nov 23, 2021
@radinamatic
Copy link
Member

Was going to suggest exactly that, thank you! 🙏🏽

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants