Skip to content
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

fix: Finish button replaced with FAB #3219

Merged
merged 19 commits into from
Oct 28, 2022

Conversation

prathamsoni11
Copy link
Contributor

@prathamsoni11 prathamsoni11 requested a review from a team as a code owner October 26, 2022 03:18
@github-actions github-actions bot added Product addition The easier it is to add a product and get Nutri-Score, Eco-Score, the happier the users. 🥫 Product page labels Oct 26, 2022
Comment on lines 95 to 98
Positioned(
child: Align(
alignment: Alignment.bottomCenter,
child: SmoothActionButtonsBar.single(
action: SmoothActionButton(
text: appLocalizations.finish,
onPressed: () async {
final LocalDatabase localDatabase =
context.read<LocalDatabase>();
final DaoProduct daoProduct =
DaoProduct(localDatabase);
final Product? localProduct =
await daoProduct.get(widget.barcode);
if (localProduct == null) {
product = Product(
barcode: widget.barcode,
);
daoProduct.put(product);
localDatabase.notifyListeners();
}
provider.set(product);
if (mounted) {
await Navigator.maybePop(context,
_isProductLoaded ? widget.barcode : null);
}
},
),
alignment: Alignment.bottomRight,
child: FloatingActionButton.extended(
Copy link
Contributor

Choose a reason for hiding this comment

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

Forget about Positioned and Align: just use the floatingActionButton parameter of SmoothScaffold.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This type of behaviour SmoothScaffold is showing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks like similar to your initial screenshot, isn't-it?

Copy link
Contributor Author

@prathamsoni11 prathamsoni11 Oct 26, 2022

Choose a reason for hiding this comment

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

Yes, it is similar but SmoothScaffold is hiding other contents on the screen. So that thing is making a difference

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here I found something to display other screen stuff by using the background colour property in SmoothScaffold as transparent

Screenshot 2022-10-26 at 7 48 56 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@monsieurtanuki @g123k should I commit this code?

Copy link
Contributor

Choose a reason for hiding this comment

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

@prathamsoni11 It looks like you removed the body from the SmoothScaffold: that would explain the empty screen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@monsieurtanuki @g123k Done it is now ready to be merged.

Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

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

Hi @prathamsoni11!
Please

  • remove the Stack as well: we don't need it anymore
  • attach a screenshot

@prathamsoni11
Copy link
Contributor Author

prathamsoni11 commented Oct 27, 2022

Hi @prathamsoni11! Please

  • remove the Stack as well: we don't need it anymore
  • attach a screenshot

@monsieurtanuki @g123k Removed Stack

Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

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

@g123k
Copy link
Collaborator

g123k commented Oct 27, 2022

Way better, but does it still overlaps the button on small screens?

@monsieurtanuki
Copy link
Contributor

Way better, but does it still overlaps the button on small screens?

@g123k I don't see why it should not: a FAB is on top of anything.
The solution for that would be to add a bottom padding to the SingleChildScrollView (cf. the standard size + position of a FAB).

@g123k
Copy link
Collaborator

g123k commented Oct 27, 2022

Way better, but does it still overlaps the button on small screens?

@g123k I don't see why it should not: a FAB is on top of anything.
The solution for that would be to add a bottom padding to the SingleChildScrollView (cf. the standard size + position of a FAB).

+1 it would even be better if we could add this padding only if needed (by computing the size)

@monsieurtanuki
Copy link
Contributor

+1 it would even be better if we could add this padding only if needed (by computing the size)

Would it?

  • If we put an extra padding on a large screen, it will be useless and will have no impact
  • If we put an extra padding on a small screen, it will be useful and will have an impact
    So the extra padding only has a positive impact (and no negative impact).

@g123k
Copy link
Collaborator

g123k commented Oct 27, 2022

If we do so, the overscroll will be ugly on large screens

@monsieurtanuki
Copy link
Contributor

If we do so, the overscroll will be ugly on large screens

I cannot picture that. Could you share a screenshot?

@g123k
Copy link
Collaborator

g123k commented Oct 27, 2022

The overscroll is this "glow" effect: https://i.stack.imgur.com/6mMn4.png

By adding a padding it will add a blank space on the bottom, which won't be nice.

@monsieurtanuki
Copy link
Contributor

The overscroll is this "glow" effect: https://i.stack.imgur.com/6mMn4.png
By adding a padding it will add a blank space on the bottom, which won't be nice.

That's what I would call a cosmetic issue, with low priority. And IMHO fixing this cosmetic issue will bring additional confusion to the code.
That said, as long as I'm not involved in it, feel free to spend your time where you find it appropriate.

@monsieurtanuki monsieurtanuki merged commit 796257f into openfoodfacts:develop Oct 28, 2022
@monsieurtanuki
Copy link
Contributor

Thank you @prathamsoni11!

@prathamsoni11
Copy link
Contributor Author

Thank you @prathamsoni11!

Your Welcome sir @monsieurtanuki 😀

@M123-dev M123-dev added this to the v4 milestone Nov 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Product addition The easier it is to add a product and get Nutri-Score, Eco-Score, the happier the users. 🥫 Product page
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants