Skip to content

[flutter_adaptive_scaffold] Support RTL #4204

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 12 commits into from
Jun 14, 2023
Merged

[flutter_adaptive_scaffold] Support RTL #4204

merged 12 commits into from
Jun 14, 2023

Conversation

ahmednfwela
Copy link
Contributor

@ahmednfwela ahmednfwela commented Jun 13, 2023 •
edited
Loading

This PR removes the hardcoded rtl directionality placed on top of AdaptiveScaffold for an unknown reason.

RTL LTR
image image

Changes in this PR

  • Core: Removed this hardcoded Directionality widget

    packages/packages/flutter_adaptive_scaffold/lib/src/adaptive_scaffold.dart

    Lines 491 to 494 in 0507297

    return Directionality(
    textDirection: TextDirection.ltr,
    child: Scaffold(
    appBar: widget.drawerBreakpoint.isActive(context) && widget.useDrawer
  • Core: Added tests to ensure text direction is passed correctly
  • Core: Modified example/adaptive_scaffold_demo.dart to demonstrate support for RTL
  • Side: There were some tests that were getting skipped, I have enabled them since their related issues landed on stable.
  • Side: Stopped ignoring prefer_const_constructors since it landed in stable as well.

I made the commit messages as descriptive as possible so that it's easier to review relevant changes.

Related issues

Question for reviewers: should the next version be 0.1.5 or 0.2.0 ?

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@ahmednfwela ahmednfwela mentioned this pull request Jun 13, 2023
Closed
gspencergoog
gspencergoog approved these changes Jun 13, 2023
View reviewed changes
Copy link
Contributor

@gspencergoog gspencergoog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

32384589-a60f0e74-c078-11e7-9bc1-e5b5287aea9d

Thanks for fixing that! We definitely shouldn't be hard coding a directionality.

@ahmednfwela
Copy link
Contributor Author

@gspencergoog what should the next version be?

@gspencergoog
Copy link
Contributor

gspencergoog commented Jun 13, 2023 •
edited
Loading

what should the next version be?

I'd say 0.1.5

justinmc
justinmc approved these changes Jun 14, 2023
View reviewed changes
Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@gspencergoog It looks like there were some formatting changes in this PR, just making sure you are ok with that.

@gspencergoog
Copy link
Contributor

It looks like there were some formatting changes in this PR, just making sure you are ok with that.

Yes, I'm OK with that. The packages repo enforces automatic formatting with dart format, so I don't worry about formatting in this repo: if the checks pass, then the formatting is correct. The indentation changes in the example markdown are fine too, since that just removes unnecessary whitespace.

@justinmc
Copy link
Contributor

Got it, thanks for the info!

@gspencergoog gspencergoog added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 14, 2023
@auto-submit auto-submit bot merged commit b7f1ad4 into flutter:main Jun 14, 2023
@ahmednfwela ahmednfwela deleted the adaptive_scaffold_rtl branch June 15, 2023 00:30
@ahmednfwela
Copy link
Contributor Author

@gspencergoog since the new version relies on dart 3/flutter 3.10, should this have been a minor version upgrade instead of a patch ?
image

@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 16, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 16, 2023
@engine-flutter-autoroll
b7f1ad4 [flutter_adaptive_scaffold] Support RTL (flutter/packages#4204)
2c0726f
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 16, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 16, 2023
@engine-flutter-autoroll
b7f1ad4 [flutter_adaptive_scaffold] Support RTL (flutter/packages#4204)
cbaaaf5
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jun 16, 2023
@engine-flutter-autoroll
Roll Packages from f9314a3 to 59d93d6 (18 revisions) (#129021)
3f73d2c 
flutter/packages@f9314a3...59d93d6

2023-06-16 [email protected] [tool] Add command aliases (flutter/packages#4207)
2023-06-16 [email protected] [google_map_flutter] Fix map object regression due to async changes (flutter/packages#4171)
2023-06-16 [email protected] [url_launcher] Add ignores for deprecated member to test (flutter/packages#4220)
2023-06-15 [email protected] Roll Flutter from 95be76a to b0188cd (10 revisions) (flutter/packages#4221)
2023-06-15 [email protected] [camera_android] Upgrading roboelectric from 4.5 to 4.10.3 (flutter/packages#4018)
2023-06-15 [email protected] [go_router] Fixes bug that GoRouterState in top level redirect doesn'â�¦ (flutter/packages#4173)
2023-06-15 [email protected] [go_router]Updates documentations around GoRouter.of, GoRouter.maybeOf, and BuildContext extension. (flutter/packages#4176)
2023-06-15 [email protected] [tool] Support code excerpts for any .md file (flutter/packages#4212)
2023-06-15 [email protected] [webview_flutter] Add support for limitsNavigationsToAppBoundDomains (flutter/packages#4026)
2023-06-15 [email protected] [webview_flutter][webview_flutter_android] Add android support for handling geolocation permissions (flutter/packages#3795)
2023-06-15 [email protected] [image_picker] add getMedia method (flutter/packages#3892)
2023-06-15 [email protected] [image_picker] getMedia platform implementations (flutter/packages#4175)
2023-06-14 [email protected] Ignore `textScaleFactor` deprecation (flutter/packages#4209)
2023-06-14 [email protected] [pigeon] Enable Obj-C integration tests in CI (flutter/packages#4215)
2023-06-14 [email protected] [flutter_adaptive_scaffold] Support RTL (flutter/packages#4204)
2023-06-14 [email protected] Manual roll Flutter from 09b7e56 to 95be76a (14 revisions) (flutter/packages#4214)
2023-06-14 [email protected] Roll Flutter (stable) from 682aa38 to 796c8ef (5 revisions) (flutter/packages#4213)
2023-06-14 [email protected] Roll Flutter from 353b8bc to 09b7e56 (17 revisions) (flutter/packages#4206)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@gspencergoog
Copy link
Contributor

gspencergoog commented Jul 5, 2023 •
edited
Loading

@ahmednfwela No, it's not adding functionality, it's a bug fix. If we consider the updated dependency a breaking change, then it would be a major version update, but since this is still a 0. release, we won't bump it to 1.0 yet.

This was referenced Jul 7, 2023
Merged
Closed
Merged
@reidbaker reidbaker mentioned this pull request Jul 26, 2023
Merged
11 tasks
@reidbaker reidbaker mentioned this pull request Aug 4, 2023
Merged
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinmc justinmc justinmc approved these changes

@gspencergoog gspencergoog gspencergoog approved these changes

Assignees
No one assigned
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: flutter_adaptive_scaffold
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

No RTL support in flutter_adaptive_scaffold package
3 participants
@ahmednfwela @gspencergoog @justinmc