Set max width to fragments #133
Conversation
To make it consistent across the app
Smaller screens use 16dp margin; anything larger will use 56dp.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds AlertDialog theming, standardizes rounded-corner drawables, restructures several dialog and fragment layouts into scrollable/centered containers with inner ConstraintLayouts and max-width constraints, and adds dialog/fragment dimension resources for responsive margins and widths. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 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. 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 |
There was a problem hiding this comment.
Pull request overview
This PR improves the layout and user experience on wide-screen devices (tablets and Android TV) by constraining the maximum width of configuration fragments and dialogs to 500dp, and standardizes dialog styling with consistent margins and rounded corners.
Changes:
- Added
dialog_horizontal_margindimension resource (16dp default, 56dp for sw600dp devices) - Applied 500dp max width constraint to Profiles, Advanced, and Change Server fragments
- Standardized dialog layouts with consistent margins, corner radius (28dp), and AlertDialogTheme usage
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| app/src/main/res/values/dimens.xml | Added horizontal margin dimension for dialogs |
| app/src/main/res/values-sw600dp/dimens.xml | Added larger horizontal margin for sw600dp devices |
| app/src/main/res/layout/fragment_server.xml | Wrapped layout in ScrollView and added 500dp max width constraint |
| app/src/main/res/layout/fragment_profiles.xml | Wrapped layout in FrameLayout and added 500dp max width constraint |
| app/src/main/res/layout/fragment_advanced.xml | Added 500dp max width constraint to existing layout |
| app/src/main/res/layout/dialog_simple_alert_message.xml | Wrapped in FrameLayout with dialog margins, improved text alignment |
| app/src/main/res/layout/dialog_confirm_change_server.xml | Wrapped in ScrollView with dialog margins |
| app/src/main/res/layout/dialog_change_server_success.xml | Wrapped in FrameLayout with dialog margins |
| app/src/main/res/drawable/bg_rounded_white.xml | Updated corner radius from 32dp to 28dp for consistency |
| app/src/main/res/drawable-night/bg_rounded_white.xml | Added missing corner radius (28dp) |
| app/src/main/java/io/netbird/client/ui/server/ChangeServerFragment.java | Applied AlertDialogTheme and programmatic width constraints to dialogs |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
app/src/main/java/io/netbird/client/ui/server/ChangeServerFragment.java
Outdated
Show resolved
Hide resolved
app/src/main/java/io/netbird/client/ui/server/ChangeServerFragment.java
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@app/src/main/res/layout/dialog_simple_alert_message.xml`:
- Around line 10-16: The ConstraintLayout dialog container currently uses
maxWidth/minWidth but will align to start inside a FrameLayout on wide screens;
update the ConstraintLayout element (the dialog container) to include
layout_gravity="center_horizontal" so it is centered when maxWidth is applied,
keeping the existing padding, background, min/max widths and wrap_content height
intact.
In `@app/src/main/res/layout/fragment_advanced.xml`:
- Around line 11-16: The ConstraintLayout uses
android:layout_width="match_parent" with android:maxWidth and
android:layout_gravity which are ignored; update the ConstraintLayout to use
constraint-based sizing by changing width to 0dp, adding
app:layout_constraintStart_toStartOf="parent" and
app:layout_constraintEnd_toEndOf="parent", and set
app:layout_constraintWidth_max="500dp" (or alternatively wrap the
ConstraintLayout in a FrameLayout with android:gravity="center_horizontal");
locate the <androidx.constraintlayout.widget.ConstraintLayout> element in
fragment_advanced.xml and apply one of these fixes so the max width and
centering work correctly.
In `@app/src/main/res/layout/fragment_server.xml`:
- Around line 130-161: Add an accessibility label to the clickable container by
setting a content description on the LinearLayout with id btn_use_netbird (e.g.,
use a new string resource like btn_use_netbird_desc) and prevent duplicate
announcements by marking the child ImageView and TextView as not individually
accessible (set android:importantForAccessibility="no" on those children) so
screen readers announce the container label only.
🧹 Nitpick comments (2)
app/src/main/res/layout/dialog_change_server_success.xml (1)
16-25: Misleading view ID:icon_warningdisplays a success icon.The ImageView uses
@drawable/check(success indicator) but is namedicon_warning. This inconsistency could confuse future maintainers.✏️ Suggested rename
<ImageView - android:id="@+id/icon_warning" + android:id="@+id/icon_success" android:layout_width="28dp" android:layout_height="28dp" android:contentDescription="@string/change_server_success_icon" android:src="@drawable/check"app/src/main/java/io/netbird/client/ui/server/ChangeServerFragment.java (1)
251-272: Consider extracting the repeated width-capping logic.
The same max-width calculation appears in both dialogs; a helper improves readability and reduces drift.♻️ Example refactor
- if (alertDialog.getWindow() != null) { - int maxWidthPx = (int) TypedValue.applyDimension( - TypedValue.COMPLEX_UNIT_DIP, 500, getResources().getDisplayMetrics()); - int screenWidth = getResources().getDisplayMetrics().widthPixels; - int width = Math.min(maxWidthPx, screenWidth); - alertDialog.getWindow().setLayout(width, ViewGroup.LayoutParams.WRAP_CONTENT); - } + applyDialogMaxWidth(alertDialog);+ private void applyDialogMaxWidth(`@NonNull` AlertDialog alertDialog) { + if (alertDialog.getWindow() == null) return; + int maxWidthPx = (int) TypedValue.applyDimension( + TypedValue.COMPLEX_UNIT_DIP, 500, getResources().getDisplayMetrics()); + int screenWidth = getResources().getDisplayMetrics().widthPixels; + int width = Math.min(maxWidthPx, screenWidth); + alertDialog.getWindow().setLayout(width, ViewGroup.LayoutParams.WRAP_CONTENT); + }
Make use of its value in fragments advanced, profile and server This also fixes a bug where fragment_advanced was not respecting the containing layout's max width value (and making switches disappear in landscape)
In order to apply layout_gravity to its children
…rLayout In order to apply layout_gravity to its children
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@app/src/main/res/layout/dialog_change_server_success.xml`:
- Around line 11-16: The ConstraintLayout in dialog_change_server_success.xml
currently uses android:layout_width="match_parent" with
android:maxWidth="@dimen/dialog_max_width", which prevents the view from
centering inside its parent; change the root ConstraintLayout's
android:layout_width to "wrap_content" so maxWidth takes effect and the dialog
can be centered (keep android:maxWidth="@dimen/dialog_max_width",
android:padding and android:background as-is).
In `@app/src/main/res/layout/dialog_confirm_change_server.xml`:
- Around line 18-23: The ConstraintLayout uses
android:layout_width="match_parent" which prevents proper centering when
combined with android:maxWidth; update the
androidx.constraintlayout.widget.ConstraintLayout element to use
android:layout_width="wrap_content" and add
android:layout_gravity="center_horizontal" so the dialog is capped by maxWidth
and centered; keep existing attributes like android:padding,
android:layout_height, android:maxWidth and android:background intact.
In `@app/src/main/res/layout/fragment_server.xml`:
- Around line 147-152: The ImageView element uses an unrelated accessibility
string (`@string/fragment_firstinstall_desc_logo`); update the ImageView's
android:contentDescription to a context-appropriate string resource (e.g.,
create/point to a server-specific string) or mark it decorative by adding
android:importantForAccessibility="no" if the parent announces the action;
modify the ImageView entry (identify by the ImageView tag with
android:src="@drawable/ic_netbird_btn") to use the chosen approach.
- Around line 9-14: The ConstraintLayout in fragment_server.xml uses
layout_width="match_parent" together with layout_gravity="center_horizontal",
which is ineffective; change the ConstraintLayout's android:layout_width from
"match_parent" to "wrap_content" so the android:maxWidth and
android:layout_gravity="center_horizontal" can center and constrain the layout
inside the parent (update the <androidx.constraintlayout.widget.ConstraintLayout
...> element accordingly).
♻️ Duplicate comments (3)
app/src/main/res/layout/dialog_simple_alert_message.xml (1)
11-17: Max width is effectively ignored withmatch_parent.
Because the container fills the parent width,android:maxWidthwon’t cap the dialog on large screens. Switch towrap_content(and center it) so the max width is honored. Based on learnings, ...🧩 Proposed fix
- <androidx.constraintlayout.widget.ConstraintLayout - android:layout_width="match_parent" + <androidx.constraintlayout.widget.ConstraintLayout + android:layout_width="wrap_content" + android:layout_gravity="center_horizontal" android:layout_height="wrap_content" android:background="@drawable/bg_rounded_nb_bg" android:maxWidth="@dimen/dialog_max_width" android:minWidth="280dp" android:padding="24dp">app/src/main/res/layout/fragment_server.xml (1)
130-162: AddcontentDescriptionto the clickable container, not the child TextView.The LinearLayout
btn_use_netbirdis focusable and clickable, so TalkBack focuses it—not the children. ThecontentDescriptionon the child TextView (line 157) won't be announced for the container. Move thecontentDescriptionto the parent LinearLayout and mark children asimportantForAccessibility="no"to avoid duplicate announcements.Suggested fix
<LinearLayout android:id="@+id/btn_use_netbird" android:layout_width="0dp" android:layout_height="wrap_content" android:orientation="horizontal" android:gravity="center" android:background="@drawable/btn_bg_transparent" android:padding="12dp" android:layout_marginTop="24dp" android:focusable="true" android:focusableInTouchMode="false" android:clickable="true" android:foreground="@drawable/focus_highlight" + android:contentDescription="@string/change_server_btn_reset" app:layout_constraintTop_toBottomOf="@id/btn_change_server" app:layout_constraintStart_toStartOf="parent" app:layout_constraintEnd_toEndOf="parent"> <ImageView android:layout_width="20dp" android:layout_height="20dp" - android:contentDescription="@string/fragment_firstinstall_desc_logo" + android:importantForAccessibility="no" android:src="@drawable/ic_netbird_btn" android:layout_marginEnd="8dp" /> <TextView android:layout_width="wrap_content" android:layout_height="wrap_content" - android:contentDescription="@string/change_server_btn_reset" + android:importantForAccessibility="no" android:text="@string/change_server_btn_reset" android:textColor="@color/nb_orange" android:textSize="16sp" android:textAllCaps="false" /> </LinearLayout>app/src/main/res/layout/fragment_advanced.xml (1)
11-20: Max width is likely not enforced; container still expands full width.
android:maxWidthisn’t honored byConstraintLayout, and amatch_parentchild won’t be centered by the parent’slayout_gravity, so the layout can still span the full screen on large devices—defeating the PR’s goal. Consider switching to constraint-based max width.🛠️ Suggested fix (constraint-based max width)
- <LinearLayout - android:layout_width="match_parent" - android:layout_height="wrap_content" - android:gravity="center_horizontal"> + <androidx.constraintlayout.widget.ConstraintLayout + android:layout_width="match_parent" + android:layout_height="wrap_content"> <androidx.constraintlayout.widget.ConstraintLayout - android:layout_width="match_parent" + android:layout_width="0dp" android:layout_height="wrap_content" - android:maxWidth="@dimen/fragment_max_width" android:padding="40dp" + app:layout_constraintStart_toStartOf="parent" + app:layout_constraintEnd_toEndOf="parent" + app:layout_constraintTop_toTopOf="parent" + app:layout_constraintWidth_max="@dimen/fragment_max_width"> ... - </LinearLayout> + </androidx.constraintlayout.widget.ConstraintLayout>Does ConstraintLayout honor android:maxWidth on match_parent, and what is the recommended way to enforce a maximum width (e.g., app:layout_constraintWidth_max with width=0dp)?Based on learnings, centering a match_parent child via layout_gravity won’t work; use wrap_content or constraints to center and cap width.
Also applies to: 607-607
🧹 Nitpick comments (3)
app/src/main/res/layout/fragment_server.xml (1)
56-71: Clickable TextView without explicitcontentDescriptionrelies on text content.The
text_setup_key_labelis clickable and focusable. TalkBack will read the visible text, but it won't convey that this is an actionable element. Consider adding acontentDescriptionthat describes the action (e.g., "Add setup key") or appending the role hint.app/src/main/res/layout/dialog_confirm_change_server.xml (1)
25-91: Minor: Indentation inconsistency inside ConstraintLayout.The child views (ImageView, TextViews, Buttons) are at the same indentation level as the
ConstraintLayoutopening tag. For clarity, consider indenting them one level deeper to visually indicate they're nested inside theConstraintLayout.app/src/main/res/layout/dialog_change_server_success.xml (1)
18-27: Renameicon_warningtoicon_successfor semantic clarity.The id
icon_warningis misleading since this dialog displays a success state with a check icon (@drawable/check). When renaming this id, also update the constraint reference at line 38:app:layout_constraintTop_toBottomOf="@id/icon_warning"to use the new name. No Kotlin/Java code changes are required.Proposed fix
<ImageView - android:id="@+id/icon_warning" + android:id="@+id/icon_success" android:layout_width="28dp" android:layout_height="28dp" android:contentDescription="@string/change_server_success_icon" android:src="@drawable/check" android:layout_margin="30dp" app:layout_constraintTop_toTopOf="parent" app:layout_constraintStart_toStartOf="parent" app:layout_constraintEnd_toEndOf="parent" />And update the constraint reference:
app:layout_constraintTop_toBottomOf="@id/icon_warning" + app:layout_constraintTop_toBottomOf="@id/icon_success"
Some configuration fragments like Profiles, Advanced and Change Server are displayed with views expanding across the entirety of the screen width for devices like tablets or Android TV.
This PR addresses that by setting a max width to those scenarios.
It also adds usage of AlertDialogTheme for dialogs, also adding consistent margins and corners.
Summary by CodeRabbit
New Features
Style
Chores
✏️ Tip: You can customize this high-level summary in your review settings.