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

trying to make all of the card layouts be in sync with one another #11573

Merged

Conversation

marcellamaki
Copy link
Member

@marcellamaki marcellamaki commented Dec 1, 2023

Summary

Following up on some regressions @thanksameeelian noticed with her work in #11515 and expanding on that work just a tiny bit for uniformity

References

Description Screenshot
"Mobile" or narrow screen: single card wide in all sections; "recent" has 3 cards per "show more" Screenshot 2023-12-01 at 11 09 15 AM
"Tablet" sized screen: 2 cards wide in all sections; "recent" has 4 cards (2x2 grid) and then adds an even number more in a 2x2 grid with "show more" Screenshot 2023-12-01 at 11 09 02 AM
"Normal" browser screen: 3 cards wide in all sections, "recent" increments rows by multiples of 3 Screenshot 2023-12-01 at 11 08 37 AM
"Narrow" browser screen: reduced side panel width (slightly different than the spec) at a higher breakpoint, such that the cards are less narrow Screenshot 2023-12-01 at 11 08 49 AM
"Wide" browser screen: Up to 4 cards, with a max grid width of 1700px Screenshot 2023-12-01 at 11 08 23 AM
Topics Page: Defaults to 4 cards/4 columns, with a max grid width of 170px Screenshot 2023-12-14 at 10 38 08 AM

|

Reviewer guidance


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

@github-actions github-actions bot added APP: Learn Re: Learn App (content, quizzes, lessons, etc.) SIZE: small labels Dec 1, 2023
@marcellamaki marcellamaki marked this pull request as ready for review December 7, 2023 19:22
@marcellamaki marcellamaki requested a review from rtibbles December 7, 2023 19:22
…op browser on Topic page to be in line with spec
@marcellamaki
Copy link
Member Author

@rtibbles this is actually ready for review now

@rtibbles
Copy link
Member

On an ultra wide screen both of these look odd, but the recent cards look odder:

image

Topic display leaves blank spaces, rather than filling them in, and instead you have to click 'show more', would be better to fill out the row:
image

Otherwise, I can't spot any other issues. The latter is far more important than the ultrawide.

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 spy an errant class - and a little cleaning up of hanging items on the topics page (if that's not too far a stretch).

@@ -124,7 +125,7 @@
:allowDownloads="allowDownloads"
data-test="search-results"
:contents="resourcesDisplayed"
:numCols="numCols"
:gridType="gridType"
Copy link
Member

Choose a reason for hiding this comment

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

As someone coming to look at this, I do find the numCols calculations significantly easier to grok in terms of intention than the rather abstract 'gridType' - I do hope we can get rid of this in future.

Copy link
Member Author

Choose a reason for hiding this comment

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

This wasn't actually an intentional doing away of numCols - the code there wasn't doing anything. I would be very happy to refactor it back to use numCols as it had been previously.

Copy link
Member

Choose a reason for hiding this comment

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

Yes - I could see that it was a no-op, as the prop didn't exist on the component. This was more of an expression of an aspiration that we use a 'desired number of columns' in future grid work!

@marcellamaki
Copy link
Member Author

Follow up for the ultrawide screens here #11653

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.

Code changes look right, manual testing checks out - it seems that the 'hanging cards' were actually a non-applicable show more! A 2 year old bug is squashed!

@marcellamaki marcellamaki merged commit 83d797d into learningequality:release-v0.16.x Dec 15, 2023
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APP: Learn Re: Learn App (content, quizzes, lessons, etc.) DEV: frontend SIZE: medium SIZE: small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants