Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 15 additions & 8 deletions openlibrary/templates/reading_goals/reading_goal_form.html
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,20 @@
<input type="hidden" name="year" value="$year_value">
$if update:
<input type="hidden" name="is_update" value="true">
<span>$_("How many books would you like to read this year?")</span>
$ 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>
<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>
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

$if not goal:
<a class="goal-form__learn-more" href="https://blog.openlibrary.org/2022/12/31/reach-your-2023-reading-goals-with-open-library" target="_blank">$_('Learn More')</a>
</div>

$if goal:
<div class="small">$_('Enter "0" to unset your reading goal. Your check-ins will be preserved.')</div>
$else:
<a class="small" href="https://blog.openlibrary.org/2022/12/31/reach-your-2023-reading-goals-with-open-library" target="_blank">$_('Learn More')</a>
<div class="goal-form__note">$_('Enter "0" to unset your reading goal. Your check-ins will be preserved.')</div>

<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>
Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?.

<button class="cta-btn cta-btn--shell reading-goal-submit-button" type="submit" data-ol-link-track="YearlyReadingGoals|SubmitGoal">$_("Submit")</button>
</div>
</form>
108 changes: 100 additions & 8 deletions static/css/components/reading-goal.less
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Original file line number Diff line number Diff line change
@@ -1,17 +1,109 @@
/* Styles for the yearly reading goal form */
.goal-form {
font-size: @font-size-title-medium;
display: flex;
flex-direction: column;
gap: 10px;
text-align: center;

button {
display: inline-block;
width: fit-content;
margin-left: 10px;
&__question {
display: flex;
flex-direction: row;
gap: 8px;
align-items: center;
justify-content: center;

span {
font-weight: 500;
}
}

&__learn-more {
font-size: @font-size-label-small;
color: @dark-grey;
text-decoration: none;

&:hover {
text-decoration: underline;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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

}

&__input-section {
display: flex;
justify-content: flex-end;
align-items: center;
gap: 15px;
padding-right: 5px;
width: 100%;

input {
width: 100px;
padding: 10px 14px;
font-size: @font-size-label-large;
text-align: center;
border: 2px solid @lighter-grey;
border-radius: 4px;
outline: none;

&:focus {
border-color: @primary-blue;
box-shadow: 0 0 0 2px fade(@primary-blue, 20%);
}
}

button {
display: inline-block;
width: fit-content;
margin: 0;
padding: 9px 20px;
font-size: @font-size-label-large;
}
}

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

Copy link

Copilot AI Oct 9, 2025

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
input {
width: 5em;
padding: 10px;
font-size: @font-size-title-medium;
&__note {
margin: 0;
color: @dark-grey;
font-size: @font-size-label-small;
text-align: center;
}
}

// Responsive design for smaller screens
@media only screen and (max-width: @width-breakpoint-tablet) {
.goal-form {
gap: 12px;

&__question {
gap: 8px;

span {
font-size: @font-size-label-medium;
}
}

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

Copilot AI Oct 9, 2025

Choose a reason for hiding this comment

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

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;

Copilot uses AI. Check for mistakes.
align-items: center;
justify-content: flex-end;
padding-right: 15px;

input {
width: 80px;
padding: 8px 12px;
font-size: @font-size-label-medium;
}

button {
padding: 8px 16px;
font-size: @font-size-label-medium;
}
}
}
}

Expand Down
Loading