fix: Improve Yearly Reading Goal modal for better usability and clarity by akramcodez · Pull Request #11343 · internetarchive/openlibrary

akramcodez · 2025-10-09T08:40:21Z

Description

This PR implements the design improvements suggested in issue #11324 to enhance the "Yearly Reading Goal" modal's usability and visual clarity.

Changes Made

Layout Improvements

Vertical Structure: Reorganized modal with clean top-to-bottom flow
Inline Question: Moved "Learn More" link to same line as question text
Right-aligned Controls: Positioned input field and submit button on the right side
Better Edit Flow: Instruction text now appears before input when editing existing goals

Typography & Styling

Larger Font Sizes: Increased input and button text from 12px to 14px for better readability
Consistent Spacing: Added proper gaps and padding throughout the modal
Enhanced Focus States: Improved input field focus indicators

Responsive Design

Mobile Optimization: Maintains horizontal layout on small screens with appropriate sizing
Consistent Experience: Same interaction patterns across all device sizes

Screenshots

Before

After

accesslint

There are accessibility issues in these changes.

accesslint · 2025-10-09T08:40:27Z

openlibrary/templates/reading_goals/reading_goal_form.html

+  <div class="goal-form__input-section">
+    $ goal_value = goal if goal else ''
+    $ min_value = 0 if goal else 1
+    <input type="number" name="goal" min="$min_value" step="1" value="$goal_value" max="10000" required>


Looks like this element is missing an accessible name or label. That makes it hard for people using screen readers or voice control to use the control.

Copilot

Pull Request Overview

This PR improves the Yearly Reading Goal modal by implementing design enhancements focused on better usability and visual clarity. The changes reorganize the modal layout and improve typography for enhanced user experience.

Restructured modal with vertical layout and improved element positioning
Enhanced typography with larger font sizes for better readability
Added responsive design optimizations for mobile devices

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
static/css/components/reading-goal.less	Complete redesign of CSS styles with flexbox layout, responsive breakpoints, and improved typography
openlibrary/templates/reading_goals/reading_goal_form.html	Restructured HTML template to support new layout with reorganized sections and conditional content placement

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-10-09T08:41:00Z

static/css/components/reading-goal.less

+  &__submit-section {
+    // This section is no longer needed as button is moved to input section
+    display: none;
  }



Instead of hiding unused sections with display: none, consider removing the CSS rule entirely or removing the corresponding HTML element. Hidden elements can cause confusion and add unnecessary code bloat.

Suggested change

&__submit-section {

// This section is no longer needed as button is moved to input section

display: none;

}

Copilot · 2025-10-09T08:41:00Z

static/css/components/reading-goal.less

+
+    &__input-section {
+      gap: 12px;
+      flex-direction: row;


The flex-direction: row property is redundant here since row is the default value for flexbox containers. This line can be removed to reduce code duplication.

Suggested change

flex-direction: row;

for more information, see https://pre-commit.ci

akramcodez · 2025-10-09T09:13:20Z

PTAL @jimchamp, thanks.

mekarpeles · 2025-10-13T21:09:49Z

@akramcodez the new designs look reasonable to us! We'll review the remaining issues shortly.

jimchamp

My understanding was that this was going to be a simple layout change. Your changes include font size and weight changes, a new hover state, etc. Staff already looked at your screenshots during our meeting today, and they approved --- so don't worry about reverting non-layout changes unless otherwise indicated. Just be sure to keep future PRs tightly scoped to the issue that they are meant to solve.

I've added some notes describing how to address the accesslint comment.

Also noting that your style changes are causing the modal to overflow on the x-axis on smaller screens in firefox:

That will have to be addressed before this can be merged.

One last thing: You mentioned Enhanced Focus States: Improved input field focus indicators as a change that you made. Can you explain how the focus indicators were improved? The improvements were not obvious to me.

jimchamp · 2025-10-14T00:26:46Z

openlibrary/templates/reading_goals/reading_goal_form.html

+  <div class="goal-form__input-section">
+    $ goal_value = goal if goal else ''
+    $ min_value = 0 if goal else 1
+    <input type="number" name="goal" min="$min_value" step="1" value="$goal_value" max="10000" required>


Suggested change

<input type="number" name="goal" min="$min_value" step="1" value="$goal_value" max="10000" required>

<input type="number" name="goal" min="$min_value" step="1" value="$goal_value" max="10000" aria-labelledby="yearly-goal-prompt" required>

This one is a little tricky, as there is no <label> element associated with the input. I think that we can use aria-labelledby here, as long as we add an id to the element containing the string How many books would you like to read this year?.

jimchamp · 2025-10-14T00:27:19Z

openlibrary/templates/reading_goals/reading_goal_form.html

-  <button class="cta-btn cta-btn--shell reading-goal-submit-button" type="submit" data-ol-link-track="YearlyReadingGoals|SubmitGoal">$_("Submit")</button>
+
+  <div class="goal-form__question">
+    <span>$_("How many books would you like to read this year?")</span>


Suggested change

<span>$_("How many books would you like to read this year?")</span>

<span id="yearly-goal-prompt">$_("How many books would you like to read this year?")</span>

Add an id here, so this can be referenced by aria-labelledby.

jimchamp · 2025-10-14T00:42:55Z

static/css/components/reading-goal.less

This is a surprising amount of additions. My understanding was that the layout was the only thing that was going to change.

jimchamp · 2025-10-14T00:46:30Z

static/css/components/reading-goal.less

+    &:hover {
+      text-decoration: underline;
+    }


What is this hover state? You didn't mention anything about this here.

Please remove this. The link is underlined to begin with.

accesslint

👏 You fixed the issue(s)! Great work.

akramcodez · 2025-10-14T05:46:02Z

@jimchamp added the changes. Sorry for the off-topic changes. Please let me know if there are any issues.

added a focus

before

after

…odal-ux

akramcodez · 2025-10-30T04:49:47Z

@jimchamp if there’s any issue, please let me know

…odal-ux

jimchamp

Sorry for the delay, @akramcodez.

I cannot merge this until the overflow issue is fixed for smaller screen sizes (mentioned here).

I still don't understand how the focus indicators were improved. Can you tell me?

for more information, see https://pre-commit.ci

akramcodez · 2025-11-05T06:17:33Z

@jimchamp The focus indicator I meant the blue border that appears when the input is active or triggered maybe I described it incorrectly earlier sorry about that.
I’ve made some changes now and I think the Firefox small-screen overflow issue is also fixed. Please take a look and let me know if there are any issues.
If the focus indicator doesn’t look good, let me know I'll remove it from the code changes.

jimchamp

I scrolled right past your "Before" picture in this post -- sorry about that! The focus indicators appear blue in production today in FF, but not in Chrome (which I didn't know).

The x-overflow issue is happening in production today, and is unrelated to these changes (again -- so sorry!).

It might make sense to rollback the previous commit if the changes were made solely for the purpose of fixing the overflow.

jimchamp · 2025-11-05T22:48:07Z

static/css/components/reading-goal.less

+  }
+}
+
+@media only screen and (max-width: 480px) {


We can't have hard-coded breakpoints. Please use one of these three

The scrolling on the x-axis is still happening on mobile devices, but it likely doesn't have anything to do with the modal. If a patron's reading log visibility is public, a Share link will appear to the right of the following counts on "My Books" pages. This link is causing the overflow (sorry about that).

It may make sense to revert the last commit if the changes were only meant to correct the overflow.

…ia query

akramcodez · 2025-11-06T08:07:31Z

@jimchamp that’s completely okay, I learned a lot from this issue. Instead of reverting that commit, I just removed the small-screen CSS and also adjusted the input and submit button sizes.

jimchamp

Thanks for your patience, @akramcodez!

Improve Yearly Reading Goal modal for better usability and clarity

9f165f3

Copilot AI review requested due to automatic review settings October 9, 2025 08:40

accesslint bot reviewed Oct 9, 2025

View reviewed changes

Copilot AI reviewed Oct 9, 2025

View reviewed changes

pre-commit-ci bot and others added 4 commits October 9, 2025 08:41

[pre-commit.ci] auto fixes from pre-commit.com hooks

a8015ce

for more information, see https://pre-commit.ci

added some changes in css

ac5f5eb

fixed css lint issue

69b4325

fixed css lint issue 2

cd469a3

jimchamp self-assigned this Oct 9, 2025

github-actions bot added the Needs: Response Issues which require feedback from lead label Oct 10, 2025

jimchamp requested changes Oct 14, 2025

View reviewed changes

fix: accessibility and styling issues in reading goal modal

d4584a8

accesslint bot reviewed Oct 14, 2025

View reviewed changes

akramcodez requested a review from jimchamp October 15, 2025 05:21

akramcodez added 2 commits October 24, 2025 10:34

Merge branch 'internetarchive:master' into fix/improve-reading-goal-m…

ebaedfc

…odal-ux

Merge branch 'internetarchive:master' into fix/improve-reading-goal-m…

637d764

…odal-ux

Merge branch 'internetarchive:master' into fix/improve-reading-goal-m…

6ffb124

…odal-ux

jimchamp requested changes Nov 4, 2025

View reviewed changes

jimchamp added Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] and removed Needs: Response Issues which require feedback from lead labels Nov 4, 2025

added some css adjustments

10ab0d7

github-actions bot removed the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Nov 5, 2025

pre-commit-ci bot and others added 3 commits November 5, 2025 05:43

[pre-commit.ci] auto fixes from pre-commit.com hooks

3a62cee

for more information, see https://pre-commit.ci

refactor: adjust reading goal component layout and remove input styles

446aef8

[pre-commit.ci] auto fixes from pre-commit.com hooks

a94cba9

for more information, see https://pre-commit.ci

github-actions bot added the Needs: Response Issues which require feedback from lead label Nov 5, 2025

jimchamp reviewed Nov 5, 2025

View reviewed changes

refactor: adjust reading goal component padding and remove mobile med…

c8143ff

…ia query

jimchamp approved these changes Nov 11, 2025

View reviewed changes

jimchamp merged commit 12d140f into internetarchive:master Nov 11, 2025
4 of 5 checks passed

	<input type="number" name="goal" min="$min_value" step="1" value="$goal_value" max="10000" required>
	<input type="number" name="goal" min="$min_value" step="1" value="$goal_value" max="10000" aria-labelledby="yearly-goal-prompt" required>

	<span>$_("How many books would you like to read this year?")</span>
	<span id="yearly-goal-prompt">$_("How many books would you like to read this year?")</span>

Uh oh!

Conversation

akramcodez commented Oct 9, 2025

Description

Changes Made

Layout Improvements

Typography & Styling

Responsive Design

Screenshots

Before

After

Uh oh!

accesslint bot left a comment

Choose a reason for hiding this comment

Uh oh!

accesslint bot Oct 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Oct 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 9, 2025

Choose a reason for hiding this comment

Uh oh!

akramcodez commented Oct 9, 2025

Uh oh!

mekarpeles commented Oct 13, 2025

Uh oh!

jimchamp left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimchamp Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

jimchamp Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

jimchamp Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

jimchamp Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

accesslint bot left a comment

Choose a reason for hiding this comment

Uh oh!

akramcodez commented Oct 14, 2025

added a focus

Uh oh!

akramcodez commented Oct 30, 2025

Uh oh!

jimchamp left a comment

Choose a reason for hiding this comment

Uh oh!

akramcodez commented Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jimchamp left a comment

Choose a reason for hiding this comment

Uh oh!

jimchamp Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

jimchamp Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

akramcodez commented Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jimchamp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

jimchamp left a comment •

edited

Loading

akramcodez commented Nov 5, 2025 •

edited

Loading

akramcodez commented Nov 6, 2025 •

edited

Loading