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

Accommodate multi-digit list markers #193

Merged
merged 1 commit into from
May 29, 2024
Merged

Conversation

jazairi
Copy link
Contributor

@jazairi jazairi commented May 20, 2024

Why these changes are being introduced:

The result list does not have a sufficient left margin to
accommodate list markers that are more than one digit. Because
of this, markers for results further down in the list are
truncated.

This issue exists on smaller viewports only. Larger viewports
have enough space for markers up to five digits. (This is the
maximum, in our case.)

Relevant ticket(s):

How this addresses that need:

This adds a left margin on smaller viewports to accommodate
4-digit result markers, and removes the result padding to provide
more room for text flow.

Side effects of this change:

  • The margin is somewhat excessive for 1- or 2-digit numbers, but
    it seems like a reasonable tradeoff for now.
  • The leading digit of 5-digit numbers is still truncated. This only
    affects one possible result, and it's unlikely that a user would get
    to result 10,000.

Developer

Accessibility
  • ANDI or WAVE has been run in accordance to our guide.
  • This PR contains no changes to the view layer.
  • New issues flagged by ANDI or WAVE have been resolved.
  • New issues flagged by ANDI or WAVE have been ticketed (link in the Pull Request details above).
  • No new accessibility issues have been flagged.
New ENV
  • All new ENV is documented in README.
  • All new ENV has been added to Heroku Pipeline, Staging and Prod.
  • ENV has not changed.
Approval beyond code review
  • UXWS/stakeholder approval has been confirmed.
  • UXWS/stakeholder review will be completed retroactively.
  • UXWS/stakeholder review is not needed.
Additional context needed to review

Try a broad search (I use 'data') and confirm that you can see all five digits of result marker 10000 on page 500.

Code Reviewer

Code
  • I have confirmed that the code works as intended.
  • Any CodeClimate issues have been fixed or confirmed as
    added technical debt.
Documentation
  • The commit message is clear and follows our guidelines
    (not just this pull request message).
  • The documentation has been updated or is unnecessary.
  • New dependencies are appropriate or there were no changes.
Testing
  • There are appropriate tests covering any new functionality.
  • No additional test coverage is required.

@mitlib mitlib temporarily deployed to timdex-ui-pi-gdt-314-tr-itwxdg May 20, 2024 17:03 Inactive
@coveralls
Copy link

coveralls commented May 20, 2024

Pull Request Test Coverage Report for Build 9276206751

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 98.653%

Totals Coverage Status
Change from base Build 9227866449: 0.0%
Covered Lines: 586
Relevant Lines: 594

💛 - Coveralls

@jazairi jazairi force-pushed the gdt-314-truncated-list-markers branch from 4ccc05c to 5b16f11 Compare May 28, 2024 20:49
Why these changes are being introduced:

The result list does not have a sufficient left margin to
accommodate list markers that are more than one digit. Because
of this, markers for results further down in the list are
truncated.

This issue exists on smaller viewports only. Larger viewports
have enough space for markers up to five digits. (This is the
maximum, in our case.)

Relevant ticket(s):

* [GDT-314](https://mitlibraries.atlassian.net/browse/GDT-314)

How this addresses that need:

This adds a left margin on smaller viewports to accommodate
4-digit result markers, and removes the result padding to provide
more room for text flow.

Side effects of this change:

* The margin is somewhat excessive for 1- or 2-digit numbers, but
it seems like a reasonable tradeoff for now.
* The leading digit of 5-digit numbers is still truncated. This only
affects one possible result, and it's unlikely that a user would get
to result 10,000.
@jazairi jazairi force-pushed the gdt-314-truncated-list-markers branch from 5b16f11 to 10a405f Compare May 28, 2024 20:53
@jazairi jazairi marked this pull request as ready for review May 28, 2024 20:55
@JPrevost JPrevost self-assigned this May 29, 2024
Copy link
Member

@JPrevost JPrevost left a comment

Choose a reason for hiding this comment

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

I wonder if this is why we had the results list numbers originally having a rule like we see removed here:
f278bb3

I'm not suggesting we change, I was just curious on the history of this type of iteration and it was interesting when I looked to see that this "bug" came out of an intentional change that didn't have the bug. Interesting.

@jazairi
Copy link
Contributor Author

jazairi commented May 29, 2024

I wonder if this is why we had the results list numbers originally having a rule like we see removed here: f278bb3

Which rule are you referring to? If it's this one, the reason we removed it is because it looks weird. It does solve this particular problem, though.

@jazairi jazairi merged commit ab1dfb6 into main May 29, 2024
5 checks passed
@jazairi jazairi deleted the gdt-314-truncated-list-markers branch May 29, 2024 14:10
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.

None yet

4 participants