Add dataset title length warning to dataset create and update form#2397
Add dataset title length warning to dataset create and update form#2397luistoptal wants to merge 9 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughA length warning feature was added for dataset title inputs, including dynamic frontend validation, accessibility improvements, and UI feedback. Supporting PHP traits, JavaScript, LESS styles, and a reusable view partial were introduced. The dataset form now uses this feature, and acceptance tests verify the warning message behavior for titles exceeding 100 characters. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant JS_Module as "length-warning.js"
participant Server
participant YiiForm as "Yii Form (TextField + LengthWarning Trait)"
User->>Browser: Loads Dataset Form
Browser->>Server: Requests form HTML
Server->>YiiForm: Renders TextField with length warning options
YiiForm->>Server: Renders _lengthWarning partial
Server-->>Browser: Returns HTML (input + warning UI)
Browser->>JS_Module: Initializes on page load
User->>Browser: Types in title field
Browser->>JS_Module: Input event detected
JS_Module->>Browser: Updates char count, toggles warning message, manages aria-describedby
Suggested labels
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
gigadb/app/client/js/length-warning.js (1)
9-13: Consider using optional chaining for cleaner code.The conditional check can be simplified using optional chaining as suggested by static analysis.
Apply this diff to use optional chaining:
-function updateCounter({ $counter, length, limit }) { - if ($counter && $counter.length) { - $counter.text(`${length} / ${limit} characters.`); - } -} +function updateCounter({ $counter, length, limit }) { + $counter?.length && $counter.text(`${length} / ${limit} characters.`); +}protected/components/controls/BaseInput.php (1)
80-87: Good refactoring to centralize ARIA attribute management.The new
appendAriaDescribedById()method properly handles existingaria-describedbyvalues and consolidates the logic. However, please address the static analysis issues.Apply this diff to fix indentation and add missing documentation:
- protected function appendAriaDescribedById($id) - { - if (isset($this->inputOptions['aria-describedby']) && trim($this->inputOptions['aria-describedby']) !== '') { - $this->inputOptions['aria-describedby'] .= " {$id}"; - } else { - $this->inputOptions['aria-describedby'] = $id; - } - } + /** + * Appends an ID to the aria-describedby attribute + * + * @param string $id The ID to append + */ + protected function appendAriaDescribedById($id) + { + if (isset($this->inputOptions['aria-describedby']) && trim($this->inputOptions['aria-describedby']) !== '') { + $this->inputOptions['aria-describedby'] .= " {$id}"; + } else { + $this->inputOptions['aria-describedby'] = $id; + } + }protected/views/shared/_lengthWarning.php (1)
1-20: Well-structured partial with proper accessibility and security measures.The ARIA attributes (
role="status",aria-live="polite") are correctly implemented for screen readers, andCHtml::encode()prevents XSS vulnerabilities. The conditional rendering logic is sound.Consider addressing the static analysis issues for consistency:
+<?php +/** + * Length Warning Partial View + * Renders dynamic character count and warning message + */ +?> <?php -$inputId = isset($inputId) ? CHtml::encode($inputId) : ''; +$inputId = isset($inputId) ? CHtml::encode($inputId) : ''; ?> <div class="length-warning-wrapper"> -<?php if ($showCount): ?> +<?php if ($showCount) : ?> <div class="length-warning-display"> - <span id="<?= $inputId ?>-length-count"><?= $initialCount ?> / <?= $threshold ?> characters.</span> + <span id="<?php echo $inputId ?>-length-count"><?php echo $initialCount ?> / <?php echo $threshold ?> characters.</span> <?php endif; ?> - <div id="<?= $inputId ?>-length-warning" role="status" aria-live="polite"> + <div id="<?php echo $inputId ?>-length-warning" role="status" aria-live="polite"> <div class="js-length-warning-message length-warning-message" style="display:none;"> <span class="fa fa-exclamation-triangle icon-warn"></span> - <?= CHtml::encode($warningMessage) ?> + <?php echo CHtml::encode($warningMessage) ?> </div> </div> -<?php if ($showCount): ?> +<?php if ($showCount) : ?> </div> <?php endif; ?> </div>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
CHANGELOG.md(1 hunks)gigadb/app/client/js/length-warning.js(1 hunks)less/modules/forms.less(1 hunks)protected/components/controls/BaseInput.php(2 hunks)protected/components/controls/TextField.php(1 hunks)protected/components/controls/traits/LengthWarning.php(1 hunks)protected/views/adminDataset/_form.php(1 hunks)protected/views/shared/_lengthWarning.php(1 hunks)tests/acceptance/AdminDatasetForm.feature(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
`**/*`: - Focus on code smells, logic errors, edge cases, missing test cases, se...
**/*: - Focus on code smells, logic errors, edge cases, missing test cases, security flaws and serious issues
- Avoid commenting on minor issues such as linting, formatting and style issues
- Never commit secrets, PII, passwords or API keys—use env-vars and .env files.
- Flag any file >300 lines or function >30 lines and suggest refactoring.
- Every new feature or bug-fix must arrive with automated tests; warn if none are touched.
- If the PR includes changes to the UI, the style guide should be updated too; warn if not.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
CHANGELOG.mdless/modules/forms.lessprotected/views/shared/_lengthWarning.phpprotected/views/adminDataset/_form.phpgigadb/app/client/js/length-warning.jsprotected/components/controls/BaseInput.phpprotected/components/controls/TextField.phptests/acceptance/AdminDatasetForm.featureprotected/components/controls/traits/LengthWarning.php
`**/*.less`: - Follow SMACSS: - Base: Default element styles - Layout: `.l-*...
**/*.less: - Follow SMACSS:
- Base: Default element styles
- Layout:
.l-*classes for page skeleton- Modules:
.my-module-*for reusable components- State:
.is-*for UI states- Theme:
.theme-*for theme overrides- Never hard-code colors, always use color variables defined in
less/base/variables.less.- Desktop-first: media queries for mobile styles.
- Limit nesting to three levels; favour flat selectors for maintainability.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
less/modules/forms.less
`**/*.php`: - HTML follows web accessibility WCAG 2 standards
**/*.php: - HTML follows web accessibility WCAG 2 standards
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
protected/views/shared/_lengthWarning.phpprotected/views/adminDataset/_form.phpprotected/components/controls/BaseInput.phpprotected/components/controls/TextField.phpprotected/components/controls/traits/LengthWarning.php
`**/*.js`: - New JavaScript must live in `giga/app/client/js/` (not in `protecte...
**/*.js: - New JavaScript must live ingiga/app/client/js/(not inprotected/js/).
- Write ES-module code (
import…export) only—no global scripts.- Functions needing >2 logical params must take a single options object.
- Rely on jQuery 3 or Vue for DOM work; avoid direct DOM APIs unless impossible.
- No Console.log
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
gigadb/app/client/js/length-warning.js
🧠 Learnings (1)
CHANGELOG.md (1)
Learnt from: rija
PR: gigascience/gigadb-website#2157
File: CHANGELOG.md:7-7
Timestamp: 2025-05-26T17:52:49.544Z
Learning: In the GigaDB project, textual changes to website content should be categorized as "Feat" (feature) entries in the CHANGELOG.md, not as "Docs" (documentation) entries, even when the changes involve updating guidance or instructional text on web pages.
🧬 Code Graph Analysis (2)
protected/views/shared/_lengthWarning.php (1)
protected/components/controls/traits/LengthWarning.php (1)
showCount(68-75)
protected/components/controls/TextField.php (2)
protected/components/controls/BaseInput.php (3)
BaseInput(3-111)renderControlGroup(89-101)init(103-110)protected/components/controls/traits/LengthWarning.php (4)
applyLengthWarningAttributes(84-101)renderLengthWarningPartial(106-125)hasLengthWarning(54-61)registerLengthWarningScript(145-155)
🪛 phpcs (3.7.2)
protected/views/shared/_lengthWarning.php
[ERROR] 2-2: Missing file doc comment
(PEAR.Commenting.FileComment.Missing)
[ERROR] 5-5: There must be a single space between the closing parenthesis and the opening brace of a multi-line IF statement; found 0 spaces
(PEAR.ControlStructures.MultiLineCondition.SpaceBeforeOpenBrace)
[ERROR] 7-7: Short PHP opening tag used with echo; expected "<?php echo $inputId ..." but found "<?= $inputId ..."
(Generic.PHP.DisallowShortOpenTag.EchoFound)
[ERROR] 7-7: Short PHP opening tag used with echo; expected "<?php echo $initialCount ..." but found "<?= $initialCount ..."
(Generic.PHP.DisallowShortOpenTag.EchoFound)
[ERROR] 7-7: Short PHP opening tag used with echo; expected "<?php echo $threshold ..." but found "<?= $threshold ..."
(Generic.PHP.DisallowShortOpenTag.EchoFound)
[ERROR] 10-10: Short PHP opening tag used with echo; expected "<?php echo $inputId ..." but found "<?= $inputId ..."
(Generic.PHP.DisallowShortOpenTag.EchoFound)
[ERROR] 13-13: Short PHP opening tag used with echo; expected "<?php echo CHtml ..." but found "<?= CHtml ..."
(Generic.PHP.DisallowShortOpenTag.EchoFound)
[ERROR] 17-17: There must be a single space between the closing parenthesis and the opening brace of a multi-line IF statement; found 0 spaces
(PEAR.ControlStructures.MultiLineCondition.SpaceBeforeOpenBrace)
protected/components/controls/BaseInput.php
[ERROR] 41-41: Line indented incorrectly; expected at least 12 spaces, found 6
(PEAR.WhiteSpace.ScopeIndent.Incorrect)
[ERROR] 42-42: Line indented incorrectly; expected 8 spaces, found 4
(PEAR.WhiteSpace.ScopeIndent.IncorrectExact)
[ERROR] 44-44: Line indented incorrectly; expected 8 spaces, found 4
(PEAR.WhiteSpace.ScopeIndent.IncorrectExact)
[ERROR] 45-45: Line indented incorrectly; expected at least 12 spaces, found 6
(PEAR.WhiteSpace.ScopeIndent.Incorrect)
[ERROR] 80-80: Line indented incorrectly; expected 4 spaces, found 2
(PEAR.WhiteSpace.ScopeIndent.IncorrectExact)
[ERROR] 80-80: Missing doc comment for function appendAriaDescribedById()
(PEAR.Commenting.FunctionComment.Missing)
[ERROR] 81-81: Line indented incorrectly; expected at least 4 spaces, found 2
(PEAR.WhiteSpace.ScopeIndent.Incorrect)
[ERROR] 82-82: Line indented incorrectly; expected 8 spaces, found 4
(PEAR.WhiteSpace.ScopeIndent.IncorrectExact)
[ERROR] 83-83: Line indented incorrectly; expected at least 12 spaces, found 6
(PEAR.WhiteSpace.ScopeIndent.Incorrect)
[ERROR] 84-84: Line indented incorrectly; expected 8 spaces, found 4
(PEAR.WhiteSpace.ScopeIndent.IncorrectExact)
[ERROR] 85-85: Line indented incorrectly; expected at least 12 spaces, found 6
(PEAR.WhiteSpace.ScopeIndent.Incorrect)
[ERROR] 86-86: Line indented incorrectly; expected 8 spaces, found 4
(PEAR.WhiteSpace.ScopeIndent.IncorrectExact)
[ERROR] 87-87: Line indented incorrectly; expected 4 spaces, found 2
(PEAR.WhiteSpace.ScopeIndent.IncorrectExact)
protected/components/controls/TextField.php
[ERROR] 27-27: Missing doc comment for class TextField
(PEAR.Commenting.ClassComment.Missing)
[ERROR] 31-31: Missing doc comment for function run()
(PEAR.Commenting.FunctionComment.Missing)
[ERROR] 34-34: Opening parenthesis of a multi-line function call must be the last content on the line
(PEAR.Functions.FunctionCallSignature.ContentAfterOpenBracket)
[ERROR] 37-37: Closing parenthesis of a multi-line function call must be on a line by itself
(PEAR.Functions.FunctionCallSignature.CloseBracketLine)
[ERROR] 40-40: Missing doc comment for function init()
(PEAR.Commenting.FunctionComment.Missing)
protected/components/controls/traits/LengthWarning.php
[ERROR] 1-1: Missing file doc comment
(PEAR.Commenting.FileComment.Missing)
[ERROR] 34-34: Missing @category tag in class comment
(PEAR.Commenting.ClassComment.MissingCategoryTag)
[ERROR] 34-34: Missing @Package tag in class comment
(PEAR.Commenting.ClassComment.MissingPackageTag)
[ERROR] 34-34: Missing @author tag in class comment
(PEAR.Commenting.ClassComment.MissingAuthorTag)
[ERROR] 34-34: Missing @license tag in class comment
(PEAR.Commenting.ClassComment.MissingLicenseTag)
[ERROR] 34-34: Missing @link tag in class comment
(PEAR.Commenting.ClassComment.MissingLinkTag)
[ERROR] 36-36: Opening brace of a trait must be on the line after the definition
(PEAR.Classes.ClassDeclaration.OpenBraceNewLine)
[ERROR] 81-81: Tag value for @param tag indented incorrectly; expected 2 spaces but found 1
(Generic.Commenting.DocComment.TagValueIndent)
[ERROR] 82-82: Tag @return cannot be grouped with parameter tags in a doc comment
(Generic.Commenting.DocComment.NonParamGroup)
[ERROR] 105-105: Missing @return tag in function comment
(PEAR.Commenting.FunctionComment.MissingReturn)
[ERROR] 144-144: Missing @return tag in function comment
(PEAR.Commenting.FunctionComment.MissingReturn)
🪛 Biome (1.9.4)
gigadb/app/client/js/length-warning.js
[error] 10-10: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 PHPMD (2.15.0)
protected/components/controls/BaseInput.php
84-86: The method appendAriaDescribedById uses an else expression. Else clauses are basically not necessary and you can simplify the code by not using them. (Clean Code Rules)
(ElseExpression)
protected/components/controls/traits/LengthWarning.php
113-113: Avoid using static access to class '\CHtml' in method 'renderLengthWarningPartial'. (Clean Code Rules)
(StaticAccess)
150-150: Avoid using static access to class '\Yii' in method 'registerLengthWarningScript'. (Clean Code Rules)
(StaticAccess)
🔇 Additional comments (10)
CHANGELOG.md (1)
7-7: LGTM! Changelog entry properly documented.The feature addition is correctly categorized as "Feat" and follows the established changelog format.
protected/views/adminDataset/_form.php (1)
465-469: Clean integration with existing form patterns.The lengthWarningOptions configuration correctly implements the 100-character threshold requirement with appropriate messaging and counter display.
less/modules/forms.less (1)
392-408: Well-structured LESS following project guidelines.The styles correctly use color variables, follow SMACSS module conventions, and implement appropriate flexbox layout for the warning display components.
tests/acceptance/AdminDatasetForm.feature (1)
621-631: Comprehensive test coverage for length warning feature.Both scenarios effectively test the 100-character threshold behavior with appropriate positive and negative test cases.
gigadb/app/client/js/length-warning.js (1)
1-63: Well-structured ES module with good accessibility support.The module correctly implements the length warning functionality with proper event delegation, accessibility management via aria-describedby, and follows the required ES module guidelines.
protected/components/controls/TextField.php (1)
25-46: Clean integration of the LengthWarning trait.The trait usage is properly implemented with correct method call ordering: applying attributes before rendering, then rendering the partial, and conditionally registering JavaScript. The separation of concerns is well maintained.
protected/components/controls/traits/LengthWarning.php (4)
54-61: Robust validation with proper null and type checking.The validation logic correctly handles edge cases and ensures the threshold is both present and non-negative.
84-101: Secure attribute application with proper array handling.The method correctly handles null input options and safely merges the required data attributes for JavaScript functionality.
106-125: Proper partial rendering with Unicode support.Using
mb_strlen()for initial count calculation ensures proper Unicode character handling, which is important for international content.
145-155: Appropriate asset management with development support.The conditional
forceCopysetting aids development workflow while the module type registration ensures proper JavaScript loading.
|
I noticed this PR has significant overlap with #2197 |
1 participant
Pull request for issue: #1028
This is a pull request for the following functionalities:
Display a warning in the dataset create / edit form title input, if the text goes beyond 100 characters
E.g: http://gigadb.gigasciencejournal.com/adminDataset/update/id/8
On page load, warning is NOT displayed
If user changes the input, warning is displayed
under 101 characters, warning is hidden
How to test?
How have functionalities been implemented?
A few new files have been added for the purpose of making this feature reusable and maintainable:
protected/components/controls/traits/LengthWarning.php
protected/views/shared/_lengthWarning.php
gigadb/app/client/js/length-warning.js
From a developer perspective, to use this warning message anywhere, simply add the corresponding options to any TextField, e.g.:
Note 1: I have added a dynamic counter, e.g.
99/100that is always shown without need to go over character limit. I believe this helps the user, plus alleviates the issue with layout shift that happens when element visibility is toggled, without the need to add an artificial empty space -- layout shift is not a dealbreaker, but I dom't like it and I think it worsens the UX.This dynamic counter is toggleable by simply removing the
showCountoptionNote 2: this feature does not affect form validation whatsoever, meaning a title longer than 100 characters will only trigger the warning, but will not prevent or interfere with form submission
Any issues with implementation?
--
Any changes to automated tests?
Added two acceptance tests to verify warning dynamic display
Summary by CodeRabbit
New Features
Style
Documentation
Tests