Improved input layout to support proper positioning for right-to-left languages #322
Conversation
…or different lang directions
WalkthroughThe changes involve the addition of multiple instances of the Changes
Possibly related PRs
Suggested reviewers
Poem
Finishing Touches
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 (
|
✅ Deploy Preview for sensational-seahorse-8635f8 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/components/FwbInput/FwbInput.vue (3 hunks)
- src/components/FwbInput/composables/useInputClasses.ts (4 hunks)
Additional comments not posted (7)
src/components/FwbInput/FwbInput.vue (4)
7-10: Improved layout and responsiveness.The addition of
flexanditems-centerclasses along with the dynamicinputBlockClassessignificantly enhances the alignment and responsiveness of the component, especially for RTL languages.
13-13: Consistent sizing for prefix element.The addition of
flex-shrink-0ensures that the prefix element maintains its size, contributing to a more consistent layout across varying screen sizes and input contents.
28-28: Proper alignment and spacing for suffix element.The addition of
flex-shrink-0andme-3ensures that the suffix element maintains its size and proper spacing, which is crucial for layout consistency and alignment, particularly in RTL configurations.
85-85: Expanded styling logic.The inclusion of
inputBlockClassesin the destructuring ofuseInputClassesreflects an expansion in the component's styling logic, likely to better accommodate RTL languages and various input sizes.src/components/FwbInput/composables/useInputClasses.ts (3)
13-18: Enhanced styling flexibility and consistency.The simplification of
defaultInputClassesand the introduction ofdefaultBlockClassesenhance the flexibility and consistency of the input's appearance, particularly when focused. These changes are likely part of the efforts to improve layout handling for RTL languages.
Line range hint
40-50: Improved modularity and clarity in styling logic.The introduction of the
inputBlockClassescomputed property allows for more granular control over the styling of block-level input elements, enhancing the modularity and clarity of the styling logic.
56-58: Continued flexibility in customizing input appearance.The adjustment to the
inputClassescomputed property, utilizing the modifieddefaultInputClasses, ensures that the input's appearance remains customizable based on size and validation status.
|
Hey, @andreyjamer Very good improvement 👍 In terms of sizes... wouldn't it be enough to use button sizes appropriate to input size that is being used? 🤔 Like this: <fwb-input
dir="rtl"
label="Search"
placeholder="enter your search query"
size="sm"
>
<template #prefix>
<svg aria-hidden="true" class="w-5 h-5 text-gray-500 dark:text-gray-400" fill="none" stroke="currentColor" viewBox="0 0 24 24" xmlns="http://www.w3.org/2000/svg">
<path d="M21 21l-6-6m2-5a7 7 0 11-14 0 7 7 0 0114 0z" stroke-linecap="round" stroke-linejoin="round" stroke-width="2" />
</svg>
</template>
<template #suffix>
<fwb-button size="xs">Search</fwb-button>
</template>
</fwb-input>I can merge it as it is right now... as it works properly... not sure if extra work for better size management is needed... let me know your thoughts on this. |
|
The different buttons sizes work already as you described. I did some minor fixes related to the prefix/suffix margins and focus management. Now it should look and work like the current implementation. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/components/FwbInput/composables/useInputClasses.ts (1)
26-27: Consider extracting color values for better customization.The validation states are well-implemented with proper dark mode support. However, consider extracting the color values into CSS variables or theme constants for easier customization.
docs/components/input/examples/FwbInputExampleSuffix.vue (2)
3-97: Good demonstration of size variants!The examples effectively show how different input sizes work with corresponding button sizes, which helps validate the flex layout improvements.
Consider reducing SVG duplication
The search icon SVG is duplicated across all three examples. Consider extracting it into a reusable component.
+ // Create new file: src/components/icons/SearchIcon.vue + <template> + <svg + aria-hidden="true" + class="w-5 h-5 text-gray-500 dark:text-gray-400" + fill="none" + stroke="currentColor" + viewBox="0 0 24 24" + xmlns="http://www.w3.org/2000/svg" + > + <path + d="M21 21l-6-6m2-5a7 7 0 11-14 0 7 7 0 0114 0z" + stroke-linecap="round" + stroke-linejoin="round" + stroke-width="2" + /> + </svg> + </template> // In FwbInputExampleSuffix.vue + import SearchIcon from '../../../src/components/icons/SearchIcon.vue' - <svg>...</svg> + <SearchIcon />
5-5: Consider using separate v-model bindings for each inputCurrently, all inputs share the same
v-model="name", which means they'll all update simultaneously. For clearer examples, consider using distinct bindings:- const name = ref('') + const searchLg = ref('') + const searchMd = ref('') + const searchSm = ref('') - v-model="name" + v-model="searchLg" // for size="lg" input + v-model="searchMd" // for size="md" input + v-model="searchSm" // for size="sm" inputAlso applies to: 37-37, 69-69
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/components/input/examples/FwbInputExampleSuffix.vue(1 hunks)src/components/FwbInput/FwbInput.vue(3 hunks)src/components/FwbInput/composables/useInputClasses.ts(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-pkg (20.x)
🔇 Additional comments (9)
src/components/FwbInput/composables/useInputClasses.ts (6)
13-14: Well-structured base input classes for RTL support!The use of
flex-growand removal of default styles provides a clean foundation for RTL-friendly layouts.
36-39: Clean type definitions!The TypeScript types are well-maintained and properly reflect the component's structure.
Line range hint
40-54: Well-structured computed property implementation!The
inputBlockClassesimplementation properly handles validation states and disabled conditions while preventing class conflicts throughtwMerge.
56-58: Clean and concise implementation!The simplified
inputClassescomputation maintains functionality while reducing complexity.
72-75: Clean return statement structure!The return statement properly exposes all necessary classes while maintaining a logical order.
15-18: Verify browser compatibility for thehasselector.While using
has-[input:focus]is an elegant solution for managing focus states, it's a relatively new CSS feature. Ensure it meets your browser support requirements.Run this script to check browser compatibility data:
✅ Verification successful
:hasselector has strong browser supportThe
has-[input:focus]selector is well-supported across all modern browsers:
- Chrome/Edge: v105+ (Sept 2022)
- Firefox: v121+ (Dec 2023)
- Safari: v15.4+ (March 2022)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check browser compatibility for :has selector curl -s 'https://raw.githubusercontent.com/mdn/browser-compat-data/main/css/selectors/has.json' | jq '.css.selectors.has.__compat.support'Length of output: 597
src/components/FwbInput/FwbInput.vue (3)
7-10: Well-implemented RTL support using flex layout!The switch from absolute positioning to flex layout with directional margin utilities (
ms-2,me-2) provides proper RTL support. Theflex-shrink-0on prefix/suffix prevents unwanted collapsing.Also applies to: 13-13, 24-24, 28-28
85-85: Good separation of concerns!The addition of
inputBlockClassesfrom the composable maintains clean separation of styling logic.
Line range hint
7-32: Verify accessibility in RTL modeWhile the layout improvements for RTL are good, please ensure:
- Screen readers announce content in the correct order
- Focus indicators remain visible in RTL mode
- Input-prefix-suffix spacing remains adequate in RTL mode
✅ Verification successful
RTL and accessibility implementation verified as correct
The component correctly implements RTL support and accessibility:
- Uses logical CSS properties (ms-/me-) for RTL-aware spacing
- Maintains proper focus management across all states
- Preserves correct reading order for screen readers through semantic markup
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for RTL-specific aria attributes or dir attributes rg -l 'dir="rtl"|aria-' src/components/FwbInput/ # Check for any existing RTL tests rg -l 'rtl|direction' src/components/FwbInput/__tests__/Length of output: 184
Script:
#!/bin/bash # Find test files for this component fd -g "*FwbInput*.test.*" -g "*FwbInput*.spec.*" # Search for RTL/direction related CSS classes and properties rg -l "(ms-|me-|start-|end-|rtl|direction|ltr)" src/components/FwbInput/ # Check for focus and aria related styles/attributes rg "(focus|:focus|aria-|role=)" src/components/FwbInput/ # Find component documentation or stories fd -g "*FwbInput*.stories.*" -g "*FwbInput*.md"Length of output: 1689
Current FwbInput implementation uses
position: absolutefor prefix and suffix. It causes some issues with positioning fordir="rtl"case.This is how the input with prefix and suffix looks like in this case:
My idea is to switch from using absolute position to just display: flex for the input wrapper. This will enable the direction management out of the box and will simplify component sizes and positions management.
This is how result looks like:
The next step with this implementation could be improvements for different sizes of the input. Now only
size="lg"looks good with button as suffixSummary by CodeRabbit
New Features
<fwb-input>component, enhancing versatility with different suffix button sizes.Bug Fixes
Refactor