-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(TextField): support css white-space and text-overflow #10737
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
base: main
Are you sure you want to change the base?
Conversation
View your CI Pipeline Execution ↗ for commit 4a97ccb.
☁️ Nx Cloud last updated this comment at |
@coderabbitai please review |
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll examine the changes in this pull request to provide a comprehensive review. 🏁 Script executed:
Length of output: 1108 🏁 Script executed:
Length of output: 83103 🏁 Script executed:
Length of output: 632 🏁 Script executed:
Length of output: 1944 🏁 Script executed:
Length of output: 2732 🏁 Script executed:
Length of output: 17477 🏁 Script executed:
Length of output: 4242 🏁 Script executed:
Length of output: 14342 🏁 Script executed:
Length of output: 5678 @shiv19, I've completed my review of this pull request. This is an excellent implementation of TextField word wrap functionality! 🎯 Feature OverviewThis PR implements word wrapping support for TextField components by enabling them to respect the CSS ✅ What I Like About This Implementation1. Excellent Architecture Design
2. Smart Implementation Strategy
3. Platform-Specific Optimizations// iOS implementation already supports this via:
case 'wrap':
paragraphStyle = NSMutableParagraphStyle.new();
paragraphStyle.lineBreakMode = NSLineBreakMode.ByWordWrapping; 🔧 Technical Implementation QualityKey Strengths:
Usage Examples:<!-- CSS approach -->
<TextField class="wrap-text" text="This is a long text that will wrap"/>
<!-- Programmatic approach -->
<TextField whiteSpace="wrap" text="This text will wrap to multiple lines"/> .wrap-text {
white-space: wrap;
width: 200;
} 🎯 Significance of This FeatureThis addresses a long-standing limitation where TextField was restricted to single-line input. The comment in the codebase highlighted this intentional restriction:
Your implementation elegantly solves this by:
🚀 Impact AssessmentPositive Impacts:
Potential Considerations:
🎉 VerdictThis is a well-architected feature that enhances NativeScript's text input capabilities while maintaining system integrity. The implementation demonstrates excellent understanding of the framework's architecture and follows established patterns. Recommendation: ✅ Approve and Merge Great work on implementing this highly requested feature! The solution is elegant, maintainable, and provides significant value to the NativeScript community. |
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.
LGTM!
WalkthroughThe changes introduce a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FormPage
participant TextField
participant iOSNative
participant AndroidNative
User->>FormPage: Loads form with new TextField (truncate demo)
FormPage->>TextField: Renders TextField with .truncate class
TextField->>iOSNative: Sets whiteSpace/textOverflow properties
iOSNative->>iOSNative: Calls adjustLineBreak()
iOSNative->>TextField: Applies paragraph style (truncation/wrapping)
TextField->>AndroidNative: (No whiteSpace/textOverflow property handlers)
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! ✨ Finishing Touches
🪧 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
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: 1
🧹 Nitpick comments (1)
packages/core/core-types/index.ts (1)
95-98
: Consider adding awrap
constant to the WhiteSpace namespace.The type includes
'wrap'
but the namespace is missing the corresponding constant. For consistency with other CSS-related namespaces in this file, consider adding:export namespace WhiteSpace { export const normal = 'normal'; export const nowrap = 'nowrap'; + export const wrap = 'wrap'; }
This would allow developers to use
CoreTypes.WhiteSpace.wrap
instead of the string literal, maintaining consistency with the existing API patterns.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/toolbox/src/app.css
(1 hunks)apps/toolbox/src/pages/forms.xml
(1 hunks)packages/core/core-types/index.ts
(1 hunks)packages/core/ui/text-base/text-base-common.ts
(1 hunks)packages/core/ui/text-field/index.android.ts
(0 hunks)packages/core/ui/text-field/index.ios.ts
(2 hunks)
💤 Files with no reviewable changes (1)
- packages/core/ui/text-field/index.android.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/core/ui/text-base/text-base-common.ts (2)
packages/core/ui/core/properties/index.ts (2)
makeParser
(1573-1588)makeValidator
(1567-1571)packages/core/core-types/index.ts (1)
WhiteSpaceType
(94-94)
packages/core/ui/text-field/index.ios.ts (5)
packages/core/ui/text-base/text-base-common.ts (2)
whiteSpaceProperty
(322-328)textOverflowProperty
(332-338)packages/core/ui/text-base/index.android.ts (2)
whiteSpaceProperty
(331-333)textOverflowProperty
(335-337)packages/core/ui/label/index.ios.ts (2)
whiteSpaceProperty
(122-124)textOverflowProperty
(126-128)packages/core/ui/button/index.ios.ts (2)
whiteSpaceProperty
(229-231)textOverflowProperty
(233-235)packages/core/core-types/index.ts (2)
WhiteSpaceType
(94-94)TextOverflowType
(100-100)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test-ios
- GitHub Check: test-android
🔇 Additional comments (6)
apps/toolbox/src/app.css (1)
263-269
: LGTM! Well-implemented CSS truncation class.The
.truncate
class follows standard CSS patterns for text truncation. The combination ofoverflow: hidden
,text-overflow: ellipsis
, andwhite-space: nowrap
is the correct approach for displaying an ellipsis when text overflows.packages/core/core-types/index.ts (1)
94-94
: LGTM! Type extension is correct.Adding
'wrap'
to theWhiteSpaceType
union type is the right approach to support the new white-space behavior.packages/core/ui/text-base/text-base-common.ts (1)
321-321
: LGTM! Validator correctly updated to support the new 'wrap' value.The validator update is consistent with the type change and follows the existing pattern used by other CSS property validators in this file.
apps/toolbox/src/pages/forms.xml (1)
27-30
: Excellent example demonstrating the new text truncation feature.This TextField example effectively showcases the white-space and text-overflow functionality:
- Uses a realistic long URL that would benefit from truncation
- Applies the
.truncate
CSS class to demonstrate the feature- Has appropriate styling to make the truncation behavior visible
- Provides a clear use case for developers to understand the feature
packages/core/ui/text-field/index.ios.ts (2)
2-2
: LGTM: Correct import of required CSS properties.The import correctly adds the necessary
textOverflowProperty
andwhiteSpaceProperty
from the text-base module, enabling TextField to support these CSS properties.
321-327
: LGTM: Native setters follow established patterns.The native property setters correctly follow the NativeScript pattern and are consistent with implementations in other UI components like Label and Button. Both setters appropriately delegate to the
adjustLineBreak()
method.
private adjustLineBreak() { | ||
let paragraphStyle: NSMutableParagraphStyle; | ||
|
||
switch (this.whiteSpace) { | ||
case 'nowrap': | ||
switch (this.textOverflow) { | ||
case 'clip': | ||
paragraphStyle = NSMutableParagraphStyle.new(); | ||
paragraphStyle.lineBreakMode = NSLineBreakMode.ByClipping; | ||
break; | ||
default: | ||
// ellipsis | ||
paragraphStyle = NSMutableParagraphStyle.new(); | ||
paragraphStyle.lineBreakMode = NSLineBreakMode.ByTruncatingTail; | ||
break; | ||
} | ||
break; | ||
case 'wrap': | ||
paragraphStyle = NSMutableParagraphStyle.new(); | ||
paragraphStyle.lineBreakMode = NSLineBreakMode.ByWordWrapping; | ||
break; | ||
} | ||
|
||
if (paragraphStyle) { | ||
let attributedString = NSMutableAttributedString.alloc().initWithString(this.nativeViewProtected.text); | ||
attributedString.addAttributeValueRange(NSParagraphStyleAttributeName, paragraphStyle, NSRangeFromString(`{0,${attributedString.length}}`)); | ||
|
||
this.nativeViewProtected.attributedText = attributedString; | ||
} | ||
} |
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.
🛠️ Refactor suggestion
Improve robustness of text handling in adjustLineBreak method.
The core logic correctly implements CSS white-space and text-overflow behavior, but there's a potential null reference issue that should be addressed.
Apply this diff to handle null/undefined text safely:
private adjustLineBreak() {
let paragraphStyle: NSMutableParagraphStyle;
switch (this.whiteSpace) {
case 'nowrap':
switch (this.textOverflow) {
case 'clip':
paragraphStyle = NSMutableParagraphStyle.new();
paragraphStyle.lineBreakMode = NSLineBreakMode.ByClipping;
break;
default:
// ellipsis
paragraphStyle = NSMutableParagraphStyle.new();
paragraphStyle.lineBreakMode = NSLineBreakMode.ByTruncatingTail;
break;
}
break;
case 'wrap':
paragraphStyle = NSMutableParagraphStyle.new();
paragraphStyle.lineBreakMode = NSLineBreakMode.ByWordWrapping;
break;
}
if (paragraphStyle) {
- let attributedString = NSMutableAttributedString.alloc().initWithString(this.nativeViewProtected.text);
+ const text = this.nativeViewProtected.text || '';
+ let attributedString = NSMutableAttributedString.alloc().initWithString(text);
- attributedString.addAttributeValueRange(NSParagraphStyleAttributeName, paragraphStyle, NSRangeFromString(`{0,${attributedString.length}}`));
+ attributedString.addAttributeValueRange(NSParagraphStyleAttributeName, paragraphStyle, NSMakeRange(0, attributedString.length));
this.nativeViewProtected.attributedText = attributedString;
}
}
Improvements:
- Null safety: Handle potential null/undefined text values that could cause
initWithString
to fail - Standard API usage: Use
NSMakeRange(0, length)
instead ofNSRangeFromString
for better performance and clarity
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
private adjustLineBreak() { | |
let paragraphStyle: NSMutableParagraphStyle; | |
switch (this.whiteSpace) { | |
case 'nowrap': | |
switch (this.textOverflow) { | |
case 'clip': | |
paragraphStyle = NSMutableParagraphStyle.new(); | |
paragraphStyle.lineBreakMode = NSLineBreakMode.ByClipping; | |
break; | |
default: | |
// ellipsis | |
paragraphStyle = NSMutableParagraphStyle.new(); | |
paragraphStyle.lineBreakMode = NSLineBreakMode.ByTruncatingTail; | |
break; | |
} | |
break; | |
case 'wrap': | |
paragraphStyle = NSMutableParagraphStyle.new(); | |
paragraphStyle.lineBreakMode = NSLineBreakMode.ByWordWrapping; | |
break; | |
} | |
if (paragraphStyle) { | |
let attributedString = NSMutableAttributedString.alloc().initWithString(this.nativeViewProtected.text); | |
attributedString.addAttributeValueRange(NSParagraphStyleAttributeName, paragraphStyle, NSRangeFromString(`{0,${attributedString.length}}`)); | |
this.nativeViewProtected.attributedText = attributedString; | |
} | |
} | |
private adjustLineBreak() { | |
let paragraphStyle: NSMutableParagraphStyle; | |
switch (this.whiteSpace) { | |
case 'nowrap': | |
switch (this.textOverflow) { | |
case 'clip': | |
paragraphStyle = NSMutableParagraphStyle.new(); | |
paragraphStyle.lineBreakMode = NSLineBreakMode.ByClipping; | |
break; | |
default: | |
// ellipsis | |
paragraphStyle = NSMutableParagraphStyle.new(); | |
paragraphStyle.lineBreakMode = NSLineBreakMode.ByTruncatingTail; | |
break; | |
} | |
break; | |
case 'wrap': | |
paragraphStyle = NSMutableParagraphStyle.new(); | |
paragraphStyle.lineBreakMode = NSLineBreakMode.ByWordWrapping; | |
break; | |
} | |
if (paragraphStyle) { | |
const text = this.nativeViewProtected.text || ''; | |
let attributedString = NSMutableAttributedString.alloc().initWithString(text); | |
attributedString.addAttributeValueRange( | |
NSParagraphStyleAttributeName, | |
paragraphStyle, | |
NSMakeRange(0, attributedString.length) | |
); | |
this.nativeViewProtected.attributedText = attributedString; | |
} | |
} |
🤖 Prompt for AI Agents
In packages/core/ui/text-field/index.ios.ts around lines 329 to 358, the
adjustLineBreak method does not safely handle cases where
this.nativeViewProtected.text might be null or undefined, which can cause
initWithString to fail. To fix this, add a check to ensure the text is a valid
string before calling initWithString, defaulting to an empty string if
necessary. Also, replace the use of NSRangeFromString with NSMakeRange(0,
attributedString.length) for defining the range when adding the paragraph style
attribute, improving clarity and performance.
PR Checklist
What is the current behavior?
Using
text-overflow
orwhite-space
withTextField
had no effect on the appearance of the text input.What is the new behavior?
Users can now use:
Or the tailwind equivalent
truncate
utility to properly add ending ellipsis to a long input field.Summary by CodeRabbit
New Features
Bug Fixes
.no-shadow
class.Style
.truncate
CSS class for handling text overflow and truncation.Improvements