feat: adding authentication page UI based on Figma #12
Conversation
|
@g123k can you review this, or ask someone knowing dart to review ? For more context you can ask @sumit-158 |
There was a problem hiding this comment.
Some comments about your code.
- don't forget to provide 2x/3x assets: https://docs.flutter.dev/development/ui/assets-and-images
| crossAxisAlignment: CrossAxisAlignment.start, | ||
| children: [ | ||
| Text( | ||
| ' Sign In to the\nCommunity Portal', |
There was a problem hiding this comment.
Do you plan to use a translation service?
Hardcoded sentences are not a good behavior
There was a problem hiding this comment.
@g123k suggestions please.
There was a problem hiding this comment.
Hello Sumit
You can have a look here
https://docs.flutter.dev/development/accessibility-and-localization/internationalization
Hope it might help you out
There was a problem hiding this comment.
Do you plan to use a translation service?
currently not , might be in future updates .
There was a problem hiding this comment.
Please ask @teolemon how to use Crowdin (the same as Smoothie)
There was a problem hiding this comment.
It's as simple as tweaking the config file in the PR, with the path to the translation file (typically *.arb for flutter)
|
@g123k I have updated the requested changes can you please check it , if its ok can you approve it else maybe give some more suggestions :) |
There was a problem hiding this comment.
Heyy @sumit-158 good code in general, just one thing I noticed. You have a lot of hardcoded values, which tends to get hard to maintain or breaks some UI when doing some color changing for example.
It already started with some SizedBoxes with a height of 30 some with 40 and other ones with 50. Same goes for colors as g123k already pointed out. In smoothie we put them all in a central location.
The colors in a Theme which we give to the MaterialApp widget so that the colors are automatically selected by the underlying widgets. And we have a single file just with global constants for spaces and how round certain corners should be,
| @@ -0,0 +1,187 @@ | |||
| // ignore_for_file: prefer_const_constructors | |||
There was a problem hiding this comment.
Constant constructors are more performant
There was a problem hiding this comment.
+1 please remove this kind of line
It's there for a reason
|
@sumit-158 Could you share the Figma file please? |
There was a problem hiding this comment.
A few more comments @sumit-158
Feed free to ask questions if my comments are not well understandable.
In general:
- Move all colors, dimensions… in a separated file
- Use a translation service
| @@ -0,0 +1,187 @@ | |||
| // ignore_for_file: prefer_const_constructors | |||
There was a problem hiding this comment.
+1 please remove this kind of line
It's there for a reason
| @override | ||
| Widget build(BuildContext context) { | ||
| return Padding( | ||
| padding: const EdgeInsets.symmetric(vertical: 30), |
There was a problem hiding this comment.
To have a consistency between all screens, could you store all dimensions in a separated file please?
There was a problem hiding this comment.
Why do you mark this as resolved, as the dimension is still hardcoded?
There was a problem hiding this comment.
how I can put in a centralized file? Is creating a const for appappding would be good in theme_constants file?
| crossAxisAlignment: CrossAxisAlignment.start, | ||
| children: [ | ||
| Text( | ||
| ' Sign In to the\nCommunity Portal', |
There was a problem hiding this comment.
Please ask @teolemon how to use Crowdin (the same as Smoothie)
|
Also please develop in a fork of this repo, not directly in it. |
https://www.figma.com/file/AFo8ZYiNoom0o2Rakzg7vd/Community-portal-(OFF-version)?node-id=0%3A1 |
|
By simplify resizing the window, I have an Overflow:
|
| @override | ||
| Widget build(BuildContext context) { | ||
| return Scaffold( | ||
| body: ListView( |
There was a problem hiding this comment.
Using a ListView for only two items is not recommanded.
Instead you should a SingleChildScrollView + Flex (which can be horizontal/vertical according to the space available)
There was a problem hiding this comment.
I couldn't change the listview, but it is responsive now.
There was a problem hiding this comment.
Please show me your code without the ListView please.
It should work without any issue here
| @override | ||
| Widget build(BuildContext context) { | ||
| return Padding( | ||
| padding: const EdgeInsets.symmetric(vertical: 30), |
There was a problem hiding this comment.
Why do you mark this as resolved, as the dimension is still hardcoded?
What
Screenshot
Fixes bug(s)
Part of