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 iOS text wrap bug #198

Merged
merged 1 commit into from
Jun 13, 2024
Merged

Fix iOS text wrap bug #198

merged 1 commit into from
Jun 13, 2024

Conversation

jazairi
Copy link
Contributor

@jazairi jazairi commented Jun 7, 2024

Why these changes are being introduced:

SwiftUI/iOS has some unusual behavior for list markers. It adds line height to them, such that when we increase the font size for a marker, the line height is dramatically increased. This is particularly visible when the text next to the marker wraps, which is common for TIMDEX UI record titles.

Relevant ticket(s):

How this addresses that need:

This implements a CSS workaround. First, we reset the line height to 0 on .result, enforcing no line height for markers. Then, we add a div to wrap the rest of the result (.result-content), with a rule that sets the line height to 1.2.

Side effects of this change:

I don't forsee any, but it's a pretty ugly workaround to accommodate a minor bug in a weird OS.

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

This will get UXWS review, but if the code reviewer would like to do QA as well, it would be useful to confirm the changes on a few browsers on desktop and on an iOS device.

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-246-io-qkvqzx June 7, 2024 13:26 Inactive
@coveralls
Copy link

coveralls commented Jun 7, 2024

Pull Request Test Coverage Report for Build 9417800516

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 9321167366: 0.0%
Covered Lines: 586
Relevant Lines: 594

💛 - Coveralls

@JPrevost
Copy link
Member

JPrevost commented Jun 7, 2024

Interesting... I wonder if our "CSS reset" settings might be expanded to cover things like this without breaking things badly:
https://github.com/MITLibraries/mitlib-style/blob/master/_assets/sass/global/_unsets.scss

@jazairi
Copy link
Contributor Author

jazairi commented Jun 7, 2024

Interesting... I wonder if our "CSS reset" settings might be expanded to cover things like this without breaking things badly: https://github.com/MITLibraries/mitlib-style/blob/master/_assets/sass/global/_unsets.scss

It would take some creativity. We might be able to do something like this:

li::before {
  line-height: 0
}
li::after {
  line-height: 1 /* or whatever */
}

@JPrevost JPrevost self-assigned this Jun 11, 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'm not sure I agree this is an improvement. I'll bring a conversation to slack tomorrow to make sure I understand what is changing and why and share what I am seeing to ensure we're seeing the same thing before we confirm if this addresses the problem

@mitlib mitlib temporarily deployed to timdex-ui-pi-gdt-246-io-rrtqhr June 12, 2024 14:33 Inactive
@jazairi jazairi temporarily deployed to timdex-ui-pi-gdt-246-io-rrtqhr June 12, 2024 16:28 Inactive
@jazairi jazairi temporarily deployed to timdex-ui-pi-gdt-246-io-rrtqhr June 12, 2024 16:31 Inactive
@jazairi jazairi requested a review from JPrevost June 12, 2024 16:32
@jazairi
Copy link
Contributor Author

jazairi commented Jun 12, 2024

@JPrevost Could you confirm that the two recently pushed commits address the issue? I'd moved the padding down a level in the markup so the hover/focus rule would include it, but in retrospect I should've just moved the hover/focus rule up a level.

@coveralls
Copy link

coveralls commented Jun 12, 2024

Pull Request Test Coverage Report for Build 9486263617

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 9321167366: 0.0%
Covered Lines: 586
Relevant Lines: 594

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 12, 2024

Pull Request Test Coverage Report for Build 9486301291

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 9321167366: 0.0%
Covered Lines: 586
Relevant Lines: 594

💛 - Coveralls

Why these changes are being introduced:

SwiftUI/iOS has some unusual behavior for list markers. It adds
line height to them, such that when we increase the font size
for a marker, the line height is dramatically increased. This
is particularly visible when the text next to the marker wraps,
which is common for TIMDEX UI record titles.

Relevant ticket(s):

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

How this addresses that need:

This implements a CSS workaround. First, we reset the line height
to 0 on `.result`, enforcing no line height for markers. Then,
we add a div to wrap the rest of the result (`.result-content`),
with a rule that sets the line height to 1.2.

Side effects of this change:

I don't forsee any, but it's a pretty ugly workaround to accommodate
a minor bug in a weird OS.
@jazairi jazairi temporarily deployed to timdex-ui-pi-gdt-246-io-rrtqhr June 12, 2024 17:04 Inactive
@jazairi
Copy link
Contributor Author

jazairi commented Jun 12, 2024

Leaving this open until it passes QA.

@coveralls
Copy link

coveralls commented Jun 12, 2024

Pull Request Test Coverage Report for Build 9486735034

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.2%) to 98.485%

Files with Coverage Reduction New Missed Lines %
app/controllers/search_controller.rb 1 98.11%
Totals Coverage Status
Change from base Build 9321167366: -0.2%
Covered Lines: 585
Relevant Lines: 594

💛 - Coveralls

@jazairi
Copy link
Contributor Author

jazairi commented Jun 13, 2024

Approved by UXWS this morning. Merging now.

@jazairi jazairi merged commit f5a2e3c into main Jun 13, 2024
5 checks passed
@jazairi jazairi deleted the gdt-246-ios-ol-bug branch June 13, 2024 13:16
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