feat(ui): floating header#115
Open
renefloor wants to merge 5 commits into
Open
Conversation
|
Warning Review limit reached
More reviews will be available in 52 minutes and 22 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds a floating mode for StreamAppBar that renders a safe-area-aware vertical gradient fade instead of a solid background+bottom border, adapts leading button styling, and exposes/app-uses theme/appStyle tokens; updates the gallery and adds unit and golden tests. ChangesFloating Mode Feature for StreamAppBar
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/design_system_gallery/lib/components/toolbar/stream_app_bar.dart (1)
100-100: ⚡ Quick winReplace hardcoded margin with spacing token.
The horizontal margin of 32 should use a spacing token from
context.streamSpacingrather than a hardcoded value. As per coding guidelines, always use spacing tokens from the theme context instead of hardcoded values.♻️ Proposed fix
margin: const EdgeInsets.symmetric(horizontal: 32), + // Replace with appropriate spacing token, e.g.: + // margin: EdgeInsets.symmetric(horizontal: spacing.xl),Note: Choose the spacing token that best matches the intended 32-pixel spacing (likely
spacing.xlor similar).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/design_system_gallery/lib/components/toolbar/stream_app_bar.dart` at line 100, Replace the hardcoded horizontal margin of 32 in StreamAppBar with the theme spacing token: locate the margin: const EdgeInsets.symmetric(horizontal: 32) in stream_app_bar.dart (the AppBar/Container/widget that sets margin) and use the spacing token from context.streamSpacing (e.g., EdgeInsets.symmetric(horizontal: context.streamSpacing.xl) or the appropriate token that maps to 32px) so the layout uses the theme spacing instead of a literal value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/design_system_gallery/lib/components/toolbar/stream_app_bar.dart`:
- Line 100: Replace the hardcoded horizontal margin of 32 in StreamAppBar with
the theme spacing token: locate the margin: const
EdgeInsets.symmetric(horizontal: 32) in stream_app_bar.dart (the
AppBar/Container/widget that sets margin) and use the spacing token from
context.streamSpacing (e.g., EdgeInsets.symmetric(horizontal:
context.streamSpacing.xl) or the appropriate token that maps to 32px) so the
layout uses the theme spacing instead of a literal value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2f9bb118-2547-48c3-ab91-fa8c2e73b2eb
📒 Files selected for processing (6)
apps/design_system_gallery/lib/components/toolbar/stream_app_bar.dartpackages/stream_core_flutter/lib/src/components/toolbar/stream_app_bar.dartpackages/stream_core_flutter/lib/src/theme/components/stream_app_bar_theme.dartpackages/stream_core_flutter/lib/src/theme/components/stream_app_bar_theme.g.theme.dartpackages/stream_core_flutter/test/components/toolbar/stream_app_bar_golden_test.dartpackages/stream_core_flutter/test/components/toolbar/stream_app_bar_test.dart
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/stream_core_flutter/lib/src/theme/stream_app_style.dart (1)
7-11: 💤 Low valueClarify naming:
StreamAppStyle.floating()keepsappBarBehaviorasregular.The
.floating()constructor setscomposerLocation = floatingbutappBarBehavior = AppBarBehavior.regular. This may confuse consumers who expect "floating" to also affect the app bar. Consider either:
- Adding a doc comment explaining this distinction, or
- Renaming to
StreamAppStyle.floatingComposer()for clarity🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/stream_core_flutter/lib/src/theme/stream_app_style.dart` around lines 7 - 11, The floating constructor StreamAppStyle.floating() sets composerLocation = ComposerLocation.floating but leaves appBarBehavior = AppBarBehavior.regular which can confuse callers; update the API to make intent explicit by either adding a doc comment on StreamAppStyle.floating() that documents that only the composerLocation changes (appBarBehavior remains regular), or rename the constructor to StreamAppStyle.floatingComposer() and update all usages to the new name so it clearly indicates only the composer is floating while composerLocation and appBarBehavior fields remain explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/stream_core_flutter/lib/src/components/toolbar/stream_app_bar.dart`:
- Around line 1-2: Remove the unused JavaScript interop import: delete the line
"import 'dart:js_interop';" from stream_app_bar.dart (the import at the top of
the file) and run the analyzer or build to confirm there are no remaining
references to dart:js_interop in this file; this file's symbols like the
StreamAppBar widget/constructor do not require JS interop so the import is safe
to remove.
---
Nitpick comments:
In `@packages/stream_core_flutter/lib/src/theme/stream_app_style.dart`:
- Around line 7-11: The floating constructor StreamAppStyle.floating() sets
composerLocation = ComposerLocation.floating but leaves appBarBehavior =
AppBarBehavior.regular which can confuse callers; update the API to make intent
explicit by either adding a doc comment on StreamAppStyle.floating() that
documents that only the composerLocation changes (appBarBehavior remains
regular), or rename the constructor to StreamAppStyle.floatingComposer() and
update all usages to the new name so it clearly indicates only the composer is
floating while composerLocation and appBarBehavior fields remain explicit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ac5ca1cf-6f3b-4b23-a425-08b5d6d077c5
📒 Files selected for processing (5)
packages/stream_core_flutter/lib/src/components/toolbar/stream_app_bar.dartpackages/stream_core_flutter/lib/src/theme.dartpackages/stream_core_flutter/lib/src/theme/stream_app_style.dartpackages/stream_core_flutter/lib/src/theme/stream_theme.dartpackages/stream_core_flutter/lib/src/theme/stream_theme.g.theme.dart
✅ Files skipped from review due to trivial changes (1)
- packages/stream_core_flutter/lib/src/theme/stream_theme.g.theme.dart
renefloor
force-pushed
the
feat/floating-header
branch
from
234103c to
24ae763
Compare
|
Actionable comments posted: 0 |
renefloor
force-pushed
the
feat/floating-header
branch
from
61b0dfb to
3bd5156
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/stream_core_flutter/lib/src/components/toolbar/stream_bottom_nav_bar.dart`:
- Around line 203-224: Wrap the floating nav item in a Semantics widget (around
the existing GestureDetector or its child) and set semantics properties to
expose it as a button with selection state: provide button: true, selected:
selected, label: item.label (and optionally a meaningful onTapHint), and include
the onTap callback via the Semantics onTap so assistive tech announces role and
selected/unselected state while preserving the existing onTap behavior; update
the build that uses item, selected, and onTap accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7a1618e8-8268-4f3e-a9c4-cd1534d3b36c
📒 Files selected for processing (4)
packages/stream_core_flutter/lib/src/components.dartpackages/stream_core_flutter/lib/src/components/scaffold/stream_scaffold.dartpackages/stream_core_flutter/lib/src/components/toolbar/stream_bottom_nav_bar.dartpackages/stream_core_flutter/lib/src/theme/stream_app_style.dart
✅ Files skipped from review due to trivial changes (1)
- packages/stream_core_flutter/lib/src/components.dart
Comment on lines
+203
to
+224
| return GestureDetector( | ||
| behavior: HitTestBehavior.opaque, | ||
| onTap: onTap, | ||
| child: Padding( | ||
| padding: EdgeInsets.symmetric(horizontal: spacing.md, vertical: spacing.xxs), | ||
| child: Column( | ||
| mainAxisSize: MainAxisSize.min, | ||
| spacing: spacing.xxxs, | ||
| children: [ | ||
| IconTheme( | ||
| data: IconThemeData(color: color, size: 20), | ||
| child: selected ? item.selectedIcon : item.icon, | ||
| ), | ||
| Text( | ||
| item.label, | ||
| style: textTheme.metadataEmphasis.copyWith(color: color), | ||
| ), | ||
| ], | ||
| ), | ||
| ), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Add semantics to the floating nav item.
The regular variant uses BottomNavigationBar, which exposes each tab to assistive tech as a selectable button with a selected state. The floating variant renders raw GestureDetector + Icon + Text, so screen readers announce only the label text — not the button role nor the selected/unselected state. Wrap the item in Semantics to match the regular variant's accessibility.
♿ Proposed fix to expose button role and selected state
- return GestureDetector(
- behavior: HitTestBehavior.opaque,
- onTap: onTap,
- child: Padding(
- padding: EdgeInsets.symmetric(horizontal: spacing.md, vertical: spacing.xxs),
- child: Column(
- mainAxisSize: MainAxisSize.min,
- spacing: spacing.xxxs,
- children: [
- IconTheme(
- data: IconThemeData(color: color, size: 20),
- child: selected ? item.selectedIcon : item.icon,
- ),
- Text(
- item.label,
- style: textTheme.metadataEmphasis.copyWith(color: color),
- ),
- ],
- ),
- ),
- );
+ return Semantics(
+ button: true,
+ selected: selected,
+ label: item.label,
+ child: GestureDetector(
+ behavior: HitTestBehavior.opaque,
+ onTap: onTap,
+ child: Padding(
+ padding: EdgeInsets.symmetric(horizontal: spacing.md, vertical: spacing.xxs),
+ child: Column(
+ mainAxisSize: MainAxisSize.min,
+ spacing: spacing.xxxs,
+ children: [
+ IconTheme(
+ data: IconThemeData(color: color, size: 20),
+ child: selected ? item.selectedIcon : item.icon,
+ ),
+ Text(
+ item.label,
+ style: textTheme.metadataEmphasis.copyWith(color: color),
+ ),
+ ],
+ ),
+ ),
+ ),
+ );📝 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.
Suggested change
| return GestureDetector( | |
| behavior: HitTestBehavior.opaque, | |
| onTap: onTap, | |
| child: Padding( | |
| padding: EdgeInsets.symmetric(horizontal: spacing.md, vertical: spacing.xxs), | |
| child: Column( | |
| mainAxisSize: MainAxisSize.min, | |
| spacing: spacing.xxxs, | |
| children: [ | |
| IconTheme( | |
| data: IconThemeData(color: color, size: 20), | |
| child: selected ? item.selectedIcon : item.icon, | |
| ), | |
| Text( | |
| item.label, | |
| style: textTheme.metadataEmphasis.copyWith(color: color), | |
| ), | |
| ], | |
| ), | |
| ), | |
| ); | |
| } | |
| return Semantics( | |
| button: true, | |
| selected: selected, | |
| label: item.label, | |
| child: GestureDetector( | |
| behavior: HitTestBehavior.opaque, | |
| onTap: onTap, | |
| child: Padding( | |
| padding: EdgeInsets.symmetric(horizontal: spacing.md, vertical: spacing.xxs), | |
| child: Column( | |
| mainAxisSize: MainAxisSize.min, | |
| spacing: spacing.xxxs, | |
| children: [ | |
| IconTheme( | |
| data: IconThemeData(color: color, size: 20), | |
| child: selected ? item.selectedIcon : item.icon, | |
| ), | |
| Text( | |
| item.label, | |
| style: textTheme.metadataEmphasis.copyWith(color: color), | |
| ), | |
| ], | |
| ), | |
| ), | |
| ), | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/stream_core_flutter/lib/src/components/toolbar/stream_bottom_nav_bar.dart`
around lines 203 - 224, Wrap the floating nav item in a Semantics widget (around
the existing GestureDetector or its child) and set semantics properties to
expose it as a button with selection state: provide button: true, selected:
selected, label: item.label (and optionally a meaningful onTapHint), and include
the onTap callback via the Semantics onTap so assistive tech announces role and
selected/unselected state while preserving the existing onTap behavior; update
the build that uses item, selected, and onTap accordingly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Submit a pull request
Linear: FLU-502
CLA
Description of the pull request
This adds the option to create a floating appbar. It automatically adjusts the background from a color to a gradient and removes the border. Also makes the back button floating by default.
Screenshots / Videos
Summary by CodeRabbit
New Features
Tests