Skip to content

Implement workdays for reminder emails#9720

Merged
ulferts merged 10 commits intodevfrom
impl/38708/workdays
Oct 4, 2021
Merged

Implement workdays for reminder emails#9720
ulferts merged 10 commits intodevfrom
impl/38708/workdays

Conversation

@oliverguenther
Copy link
Copy Markdown
Member

@oliverguenther oliverguenther force-pushed the impl/38708/workdays branch 2 times, most recently from e836457 to 20716a7 Compare September 30, 2021 15:11
Copy link
Copy Markdown
Contributor

@ulferts ulferts left a comment

Choose a reason for hiding this comment

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

Mostly just nitpicking in the code.

Continuing nitpicking, the margins

  • between the heading and the first checkbox
  • between the checkboxes
  • to the left of the checkboxes (although I don't understand where these should be indented)

image

are way smaller than in the specification. Those styling changes can also be done in a separate PR where we give the whole page another polish.

The functionality itself is looking solid.

Comment thread app/models/users/scopes/having_reminder_mail_to_send.rb Outdated
Comment thread config/schemas/user_preferences.schema.json
Comment thread spec/contracts/user_preferences/update_contract_spec.rb
Comment thread spec/features/notifications/reminder_mail_spec.rb
@oliverguenther
Copy link
Copy Markdown
Member Author

I fixed everything except for the margins, as I'd also vote for a separate PR that does the styling as in here I just want to use the existing components

@ulferts ulferts merged commit 4f1813e into dev Oct 4, 2021
@ulferts ulferts deleted the impl/38708/workdays branch October 4, 2021 07:35
akabiru added a commit that referenced this pull request Apr 15, 2026
The idLabel getter was bypassing formattedId entirely, always rendering
the numeric PK with a # prefix. In semantic mode this showed #9720
instead of KSTP-1.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants