-
Notifications
You must be signed in to change notification settings - Fork 6.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: form required style adjustment #4668
Conversation
|
WalkthroughThe pull request introduces modifications to three Vue components: Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/effects/layouts/src/authentication/authentication.vue (1)
64-71
: Improved conditional rendering for logo and app nameThe changes effectively address the layout issues by introducing conditional rendering for the logo and app name section. This enhancement prevents unnecessary DOM elements when both logo and app name are absent, improving the component's efficiency.
Consider adding an
aria-label
to the logo image for better accessibility:- <img v-if="logo" :alt="appName" :src="logo" class="mr-2" width="42" /> + <img v-if="logo" :alt="appName" :src="logo" class="mr-2" width="42" :aria-label="`${appName} logo`" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/@core/ui-kit/form-ui/src/form-render/form-field.vue (1 hunks)
- packages/@core/ui-kit/form-ui/src/form-render/form-label.vue (1 hunks)
- packages/effects/layouts/src/authentication/authentication.vue (1 hunks)
🧰 Additional context used
🔇 Additional comments (5)
packages/@core/ui-kit/form-ui/src/form-render/form-label.vue (4)
3-3
: LGTM: Import ofcn
utility function.The addition of the
cn
import is appropriate for the class name concatenation used in the template.
6-6
: Excellent addition of theclass
prop.The new optional
class
property enhances the component's flexibility, allowing for custom styling from parent components. This change improves the reusability of theFormLabel
component and aligns well with the PR's objective of adjusting form styles.
11-11
: Good: Improved props declaration for template usage.The change from
defineProps<Props>()
toconst props = defineProps<Props>()
is appropriate. It allows for the use ofprops
within the template, which is necessary for the subsequent template modifications. This change enhances the component's internal structure without altering its behavior.
15-17
: Excellent: Template improvements address the PR objective.The template changes effectively solve the issue of required field indicator positioning:
- The use of the
cn
function in the class attribute (line 15) allows for flexible styling, combining default and custom classes.- Moving the required asterisk before the slot content (lines 16-17) ensures consistent positioning of the indicator, addressing the main objective of this PR.
These modifications improve the component's structure and functionality while maintaining the conditional rendering of both the required asterisk and the help tooltip. Great job on addressing the issue described in #4661!
packages/@core/ui-kit/form-ui/src/form-render/form-field.vue (1)
281-281
: LGTM: Improved label alignment for horizontal layoutsThe addition of the 'justify-end' class when
!isVertical
is true improves the alignment of labels in horizontal layouts. This change addresses the inconsistency mentioned in issue #4661 by ensuring that labels (and consequently, required field indicators) are properly aligned to the right in horizontal layouts.This modification:
- Doesn't affect vertical layouts, preserving their current behavior.
- Improves consistency between horizontal and vertical layouts.
- Aligns with the PR objectives to fix the positioning of required field indicators.
Description
fixed #4661
Type of change
Please delete options that are not relevant.
pnpm-lock.yaml
unless you introduce a new test example.Checklist
pnpm run docs:dev
command.pnpm test
.feat:
,fix:
,perf:
,docs:
, orchore:
.Summary by CodeRabbit
New Features
FormField
andFormLabel
components for improved layout and flexibility.Bug Fixes
FormField
component.Refactor