fixed problems with * for required#1673
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe legacy form renderer's renderThemeForm() now outputs the required-field marker ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1673 +/- ##
=============================================
+ Coverage 0 19.19% +19.19%
- Complexity 0 7450 +7450
=============================================
Files 0 620 +620
Lines 0 39087 +39087
=============================================
+ Hits 0 7501 +7501
- Misses 0 31586 +31586 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@htdocs/class/xoopsform/renderer/XoopsFormRendererLegacy.php`:
- Around line 659-661: Fix the PSR-12 indentation in the colspan branch of
XoopsFormRendererLegacy by replacing tabs with spaces to match surrounding code
formatting around the conditional that appends the required marker (the
$ele->isRequired() check that adds '<span class="caption-marker">*</span>' to
$ret); locate the colspan branch in class XoopsFormRendererLegacy and adjust
indentation for that if-block so it uses the same space-based indentation as the
other branch.
- Around line 645-647: The new conditional that appends the required asterisk
uses tab characters instead of the project's PSR-12 space indentation; update
the indentation in the XoopsFormRendererLegacy class around the block that
checks $ele->isRequired() and appends to $ret so spaces are used to match
surrounding code (preserve the logic that adds '<span
class="caption-marker">*</span>' only when $ele->isRequired() is true).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 87ad059c-b6e4-4f4d-8a20-e4aeeb5b9208
📒 Files selected for processing (1)
htdocs/class/xoopsform/renderer/XoopsFormRendererLegacy.php
There was a problem hiding this comment.
Pull request overview
Fixes the legacy theme form renderer so the required-field * marker is only shown for elements that are actually required, improving correctness of form UI output.
Changes:
- Wrap required
*caption marker output in anisRequired()conditional (for both normal andnocolspanrendering paths).
| if ($ele->isRequired()) { | ||
| $ret .= '<span class="caption-marker">*</span>'; | ||
| } |
There was a problem hiding this comment.
Newly added block is indented with tabs, which breaks the file’s PSR-12/space-based indentation and makes diffs noisy. Please replace the tab indentation with spaces to match surrounding code.
| if ($ele->isRequired()) { | ||
| $ret .= '<span class="caption-marker">*</span>'; | ||
| } |
There was a problem hiding this comment.
This added if ($ele->isRequired()) block uses tab indentation; please use spaces so indentation matches surrounding code style.
| if (($caption = $ele->getCaption()) != '') { | ||
| $ret .= '<div class="xoops-form-element-caption' . ($ele->isRequired() ? '-required' : '') . '">'; | ||
| $ret .= '<span class="caption-text">' . $caption . '</span>'; | ||
| $ret .= '<span class="caption-marker">*</span>'; | ||
| if ($ele->isRequired()) { | ||
| $ret .= '<span class="caption-marker">*</span>'; | ||
| } |
There was a problem hiding this comment.
This change alters the HTML output for theme forms (required * marker now conditional). There are existing unit tests for XoopsThemeForm::render(); please add/adjust tests to assert that caption-marker is present only for required elements and absent for non-required ones, to prevent regressions.
…elements Add 4 tests to XoopsThemeFormTest verifying that the caption-marker asterisk span is rendered only for required elements and absent for non-required ones, covering both the normal and nocolspan rendering paths.
|
|
@ggoffy Thank you! |
Summary by CodeRabbit