Skip to content
This repository has been archived by the owner on Jun 13, 2024. It is now read-only.

Added Cupertino Context Menu in cupertino demo section . #361

Merged
merged 16 commits into from
Oct 27, 2020
Merged

Added Cupertino Context Menu in cupertino demo section . #361

merged 16 commits into from
Oct 27, 2020

Conversation

Alabhya268
Copy link
Contributor

Is it ok or anything needs to be changed ?

@Alabhya268 Alabhya268 changed the title Added Cupertino Context Menu demo in cupertino section . Added Cupertino Context Menu in cupertino demo section . Oct 26, 2020
@Alabhya268
Copy link
Contributor Author

@rami-a @clocksmith Please have a look at this PR .

@Alabhya268 Alabhya268 marked this pull request as ready for review October 26, 2020 07:29
@rami-a
Copy link
Contributor

rami-a commented Oct 26, 2020

Could you please include a screenshot of the demo in action, thanks!

@Alabhya268
Copy link
Contributor Author

Alabhya268 commented Oct 26, 2020

Could you please include a screenshot of the demo in action, thanks!

3
2

Please have a look at these screenshots .

@Alabhya268
Copy link
Contributor Author

Alabhya268 commented Oct 26, 2020

@rami-a , I just noticed a bug . When app is in dark theme mode ,The text below the flutter icon is not showing up in context menu demo ,it should show the text as shown in above screenshots .
bug-2

final galleryLocalizations = GalleryLocalizations.of(context);
return CupertinoPageScaffold(
navigationBar: CupertinoNavigationBar(
middle: Text(galleryLocalizations.demoCupertinoContextMenuTitle),
Copy link
Contributor

Choose a reason for hiding this comment

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

Add automaticallyImplyLeading: false so the back button doesn't appear

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

"@demoCupertinoContextMenuSubtitle": {
"description": "Subtitle for the cupertino context menu component demo."
},
"demoCupertinoContextMenuDescription": "A full-screen modal route that opens when the child is long-pressed. When open, the CupertinoContextMenu shows the child, or the widget returned by previewBuilder if given, in a large full-screen Overlay with a list of buttons specified by actions.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Simplify this description a bit . Something like:

"An iOS-style full screen contextual menu that appears when an element is long-pressed."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've replaced it with this one .

@rami-a
Copy link
Contributor

rami-a commented Oct 26, 2020

@rami-a , I just noticed a bug . When app is in dark theme mode ,The text below the flutter icon is not showing up in context menu demo ,it should show the text as shown in above screenshots .
bug-2

Hmm, this is odd, but should be fixable

final galleryLocalizations = GalleryLocalizations.of(context);
return CupertinoPageScaffold(
navigationBar: CupertinoNavigationBar(
middle: Text(galleryLocalizations.demoCupertinoContextMenuTitle),
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a trailing comma to format nicer

@Alabhya268
Copy link
Contributor Author

@rami-a

@rami-a , I just noticed a bug . When app is in dark theme mode ,The text below the flutter icon is not showing up in context menu demo ,it should show the text as shown in above screenshots .
bug-2

Hmm, this is odd, but should be fixable

should I manually hard-code the text color here ?

@rami-a
Copy link
Contributor

rami-a commented Oct 27, 2020

Yes, feel free to manually hard code the text color to black here. Additionally, could you center align the text. Then it should be good to merge, thanks!

Copy link
Contributor

@rami-a rami-a left a comment

Choose a reason for hiding this comment

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

Thanks again for another great demo, looks good but just a few comments

child: Container(
child: const FlutterLogo(size: 250),
),
actions: <Widget>[
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the <Widget>

lib/l10n/intl_en.arb Outdated Show resolved Hide resolved
@rami-a rami-a merged commit 0544532 into flutter:master Oct 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants