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: Login button whole width centre #2668

Merged

Conversation

prathamsoni11
Copy link
Contributor

What

Screenshot

Screenshot_2022-07-27-20-58-24-663_org openfoodfacts scanner

Fixes bug(s)

Part of

@prathamsoni11 prathamsoni11 requested a review from a team as a code owner July 27, 2022 15:30
@AshAman999
Copy link
Member

Few comments for you that might be helpful

  • I don't know why you create a pr then close it then open a new one, try to work on one, if you could revisit your git concepts
  • 2nd the tests are failing, probably theme color, did you look into if the app still looks good in both light and dark mode
    ( probably button theme color)
  • You can test your code locally by running flutter test

@prathamsoni11
Copy link
Contributor Author

Few comments for you that might be helpful

  • I don't know why you create a pr then close it then open a new one, try to work on one, if you could revisit your git concepts
  • 2nd the tests are failing, probably theme color, did you look into if the app still looks good in both light and dark mode
    ( probably button theme color)
  • You can test your code locally by running flutter test
  • Sorry for deleting PR actually my project was crashed and I was not able to commit so that's why I have to do that. There is git issue in Android Studio sometime it show there is no git and sometime it runs correctly so due to that I did this.
  • Ok let me check in every theme mode.

@AshAman999
Copy link
Member

Hey @prathamsoni11
Don't stress about the tests, probably not a problem by your side, I guess the tests need to updated here now

@prathamsoni11
Copy link
Contributor Author

Hey @prathamsoni11
Don't stress about the tests, probably not a problem by your side, I guess the tests need to updated here now

ok

@AshAman999
Copy link
Member

@monsieurtanuki
Can you have a look, if the golden tests needs to be updated here

@monsieurtanuki
Copy link
Contributor

Can you have a look, if the golden tests needs to be updated here

@AshAman999 Not really. Just run flutter test --update-goldens - if there are file changes, then they need to be updated.

Btw @prathamsoni11 I'm still waiting for answers and for a screenshot on tablets.

@prathamsoni11
Copy link
Contributor Author

Can you have a look, if the golden tests needs to be updated here

@AshAman999 Not really. Just run flutter test --update-goldens - if there are file changes, then they need to be updated.

Btw @prathamsoni11 I'm still waiting for answers and for a screenshot on tablets.

Can I send the screenshot of web version?

@monsieurtanuki
Copy link
Contributor

Can I send the screenshot of web version?

There's no web version.
Just use device_preview, on the bottom of your device/emulator.

@prathamsoni11
Copy link
Contributor Author

Can I send the screenshot of web version?

There's no web version. Just use device_preview, on the bottom of your device/emulator.

ok sending

@prathamsoni11
Copy link
Contributor Author

prathamsoni11 commented Jul 28, 2022

@monsieurtanuki
Screenshot_2022-07-28-17-59-03-233_org openfoodfacts scanner

@monsieurtanuki
Copy link
Contributor

@M123-dev Does the screenshot on tablet match your expectations?

@M123-dev
Copy link
Member

Yes it's great that it's in center. Maybe we could put it as a trailing on big devices, but as the whole UI is aimed at mobile users we don't need to do that here

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.

@prathamsoni11 I'm afraid that your code works only on the main settings page. But not in the account settings page.

if (type == PreferencePageType.ACCOUNT) {
children.add(
Container(
margin: const EdgeInsets.symmetric(horizontal: LARGE_SPACE * 7),
Copy link
Contributor

Choose a reason for hiding this comment

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

It's probably 6 rather than 7. Or 8, I don't know.
Or maybe a Center would do the trick, wouldn't it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe 6 would be better

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 6. For instance use something related to the screen width.

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

@prathamsoni11
Copy link
Contributor Author

@prathamsoni11 I'm afraid that your code works only on the main settings page. But not in the account settings page.

Sorry I cannot get the page you are referring about. Can you justify it more?

@monsieurtanuki
Copy link
Contributor

Sorry I cannot get the page you are referring about. Can you justify it more?

I thought the button was used somewhere else. It's not, you're right, forget about that.

@prathamsoni11
Copy link
Contributor Author

prathamsoni11 commented Jul 28, 2022

@monsieurtanuki
Which one I should commit? I used screen width as reference.

Screenshot_2022-07-28-19-25-31-265_org openfoodfacts scanner
Screenshot_2022-07-28-19-25-23-144_org openfoodfacts scanner

@monsieurtanuki
Copy link
Contributor

@prathamsoni11 For the record I have very little interest in UI in general, and both screenshots look good.
That said, it's safer to use a larger button - you never know how "sign in" is going to be localized - so the first screenshot.
I'm even surprised that you need to specify a width.

@prathamsoni11
Copy link
Contributor Author

@prathamsoni11 For the record I have very little interest in UI in general, and both screenshots look good.
That said, it's safer to use a larger button - you never know how "sign in" is going to be localized - so the first screenshot.
I'm even surprised that you need to specify a width.

done with commit

@monsieurtanuki
Copy link
Contributor

Just run flutter test --update-goldens - if there are file changes, then they need to be committed.

@prathamsoni11 ping

@prathamsoni11
Copy link
Contributor Author

flutter test --update-goldens

All test passed 👍🏻

@monsieurtanuki
Copy link
Contributor

All test passed 👍🏻

Of course they did: those "tests" generate the screenshots we have to test against.
You (I mean the command) have generated files: please commit them.

@prathamsoni11
Copy link
Contributor Author

All test passed 👍🏻

Of course they did: those "tests" generate the screenshots we have to test against. You (I mean the command) have generated files: please commit them.

done

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.

Thank you @prathamsoni11!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants