Conversation
…ncements Upgraded Bootstrap from version 5.3.3 to 5.3.8 in package.json and updated the browserslist configuration to include support for the last two major versions of various browsers. Additionally, added a cleanup step in the CSS build process to improve asset management.
Updated the _custom_layout.scss and _custom.scss files to replace specific font families with a CSS variable for consistency. Introduced a new password visibility controller in JavaScript to enhance user experience by allowing users to toggle password visibility in forms. This change improves both styling and functionality across the application.
Updated the login form to include a password visibility toggle feature, improving user experience by allowing users to view their password while typing. Additionally, retained the user's email input by pre-filling it from the parameters or resource, enhancing usability during login attempts.
Enhanced the Payment model by adding a method to validate the format of the transaction date. The new method checks if the date is in the correct 'MM/DD/YYYY' format and adds an error message if it is not. This change improves data integrity by ensuring only valid transaction dates are processed.
…add CSS cleanup script Modified the application layout to conditionally load JavaScript and CSS assets based on the environment, improving development efficiency. Additionally, introduced a new script to clean up the application CSS by removing unnecessary styles, enhancing the overall asset management process.
…yout Updated the password visibility controller to improve clarity in variable naming and logic for toggling password visibility. Additionally, simplified the application layout by removing environment-specific asset loading conditions, ensuring consistent asset management across all environments. Enhanced error handling in the CSS cleanup script to improve robustness.
Updated the _custom.scss file to adjust the order of Bootstrap imports, ensuring that custom variable overrides are applied correctly. This change enhances the consistency of styling by allowing custom definitions to take precedence over Bootstrap defaults.
There was a problem hiding this comment.
Pull request overview
This PR updates front-end styling/build configuration and the login experience, while also adjusting a payment model validation.
Changes:
- Bump Bootstrap to
^5.3.8, adjust Browserslist targets, and add a post-build CSS cleanup step (bin/cleanup_application_css.js). - Update login UI: preserve entered email on re-render and add a Stimulus-powered password visibility toggle; disable Rails preload link headers in the application layout.
- Refine
Payment#valid_transaction_datevalidation logic to prefer%m/%d/%Yparsing with a fallback.
Reviewed changes
Copilot reviewed 8 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| yarn.lock | Updates locked versions (notably Bootstrap and caniuse-lite). |
| package.json | Bumps Bootstrap, adds CSS cleanup script to build pipeline, updates Browserslist queries. |
| bin/cleanup_application_css.js | New Node script to post-process generated application.css. |
| app/views/layouts/application.html.erb | Disables preload_links_header for JS/CSS tags. |
| app/views/devise/sessions/new.html.erb | Preserves email input; adds password visibility UI and Stimulus bindings. |
| app/models/payment.rb | Changes transaction date validation/parsing behavior. |
| app/javascript/controllers/password_visibility_controller.js | New Stimulus controller for password visibility toggle (and trimming). |
| app/javascript/controllers/index.js | Registers the new Stimulus controller. |
| app/assets/stylesheets/_custom_layout.scss | Switches typography to Bootstrap body font CSS var. |
| app/assets/stylesheets/_custom.scss | Reorders Bootstrap imports and adds a font-family override before variables. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <%= f.password_field :password, | ||
| autocomplete: 'current-password', | ||
| class: 'form-control pe-5', | ||
| data: { password_visibility_target: 'input', action: 'blur->password-visibility#trimWhitespace' } %> |
There was a problem hiding this comment.
trimWhitespace modifies the password value on blur/submit. Passwords can legitimately contain leading/trailing spaces (Devise treats them as significant), so trimming can prevent some users from authenticating even with the correct password. Consider removing the trimming behavior, or only trimming when you can guarantee passwords never contain surrounding whitespace (and ideally handle that server-side consistently).
| data: { password_visibility_target: 'input', action: 'blur->password-visibility#trimWhitespace' } %> | |
| data: { password_visibility_target: 'input' } %> |
| trimWhitespace() { | ||
| this.inputTarget.value = this.inputTarget.value.trim() | ||
| } |
There was a problem hiding this comment.
trimWhitespace() alters the password field value (and is bound to form submit). Since passwords may validly include leading/trailing whitespace, this can cause correct credentials to fail. Safer approach is to avoid mutating password input and limit this controller to visibility toggling only.
| def valid_transaction_date | ||
| return if transaction_date.blank? | ||
|
|
||
| return if valid_transaction_date_format?(transaction_date) | ||
|
|
||
| errors.add(:transaction_date, "must be a valid date") | ||
| end | ||
|
|
||
| def valid_transaction_date_format?(value) | ||
| date_string = value.to_s.strip | ||
| return false if date_string.blank? | ||
|
|
||
| Date.strptime(date_string, '%m/%d/%Y') | ||
| true | ||
| rescue ArgumentError | ||
| begin | ||
| Date.parse(transaction_date) | ||
| Date.parse(date_string) | ||
| true | ||
| rescue ArgumentError | ||
| errors.add(:transaction_date, "must be a valid date") | ||
| false | ||
| end |
There was a problem hiding this comment.
valid_transaction_date behavior changed (whitespace stripping + preferred %m/%d/%Y parsing with fallback). spec/models/payment_spec.rb currently has no coverage for transaction_date validation, so regressions/format edge cases won’t be caught. Please add model specs for valid/invalid dates (including whitespace and ambiguous formats) to lock in the intended behavior.
| "build": "esbuild app/javascript/*.* --bundle --sourcemap --format=esm --outdir=app/assets/builds --public-path=/assets", | ||
| "build:css:compile": "sass ./app/assets/stylesheets/application.bootstrap.scss:./app/assets/builds/application.css --no-source-map --load-path=node_modules", | ||
| "build:css:prefix": "postcss ./app/assets/builds/application.css --use=autoprefixer --output=./app/assets/builds/application.css", | ||
| "build:css": "yarn build:css:compile && yarn build:css:prefix", | ||
| "build:css:cleanup": "node ./bin/cleanup_application_css.js", | ||
| "build:css": "yarn build:css:compile && yarn build:css:prefix && yarn build:css:cleanup", | ||
| "watch:css": "nodemon --watch ./app/assets/stylesheets/ --ext scss --exec \"yarn build:css\"" |
There was a problem hiding this comment.
The PR description only mentions reordering Bootstrap imports in _custom.scss, but this PR also changes dependencies (Bootstrap bump), build scripts (adds CSS cleanup step + browserslist change), view behavior (disables preload link headers, adds password toggle UI), and model validation logic. Please update the PR description to reflect the full scope or split these unrelated changes into separate PRs to make review/risk management easier.
Updated the _custom.scss file to adjust the order of Bootstrap imports, ensuring that custom variable overrides are applied correctly. This change enhances the consistency of styling by allowing custom definitions to take precedence over Bootstrap defaults.