Skip to content

[Feature/PE1-16] Implementation of the welcome page#11

Open
Sheldonsu28 wants to merge 4 commits intodevelopfrom
feature/PE1-16-welcome-page
Open

[Feature/PE1-16] Implementation of the welcome page#11
Sheldonsu28 wants to merge 4 commits intodevelopfrom
feature/PE1-16-welcome-page

Conversation

@Sheldonsu28
Copy link
Member

This contains the implementation for the welcome page.

@polyteam-bot
Copy link
Member

Scope of work:

https://polyteam.atlassian.net/browse/PE1-16.

@Sheldonsu28 Sheldonsu28 added the automerge Automatically merge pull request when it's ready label Jun 19, 2021
@jimmy-lan
Copy link
Member

Looks like solid progress, thanks @Sheldonsu28!

Given Frank's asset management branch is recently merged, would you mind syncing this branch with #7?

Just run git merge develop from your branch locally and move the assets to a good place.

@shoko-inori
Copy link
Member

It appears to me that conflict of packages do exists; test_coverage ^0.4.2 is incompatible with flutter_launcher_icons ^0.9.0, resulting in flutter failing to run.

Copy link
Member

@shoko-inori shoko-inori left a comment

Choose a reason for hiding this comment

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

Version conflict on installed packages needs to be resolved.

@Sheldonsu28 Sheldonsu28 requested a review from shoko-inori June 25, 2021 22:25
Copy link
Member

@jimmy-lan jimmy-lan left a comment

Choose a reason for hiding this comment

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

Thanks for contributing! Just a few comments for us to think about. Please see below.


Some general notes about file naming

Regarding file naming, please read this section of the dart language docs: https://dart.dev/guides/language/effective-dart/style#do-name-libraries-and-source-files-using-lowercase_with_underscores.

In general, let's have a discussion on the convention above. According to the above document, we should name files using underscores rather than camel case. For example, maybe we should name a file calender_pic.dart instead of calenderPic.dart.

This is great work. However, it takes some time for all of us to get familiar with the Dart language. From this pull request, I think we are missing some convention checks around file names & image location. I will add that to my next tasks. Thank you for bringing this up!

@@ -0,0 +1,56 @@
import 'package:flutter/material.dart';

class BottomSquareWithButton extends StatelessWidget {
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for your contribution! I was wondering if there's a better name for this widget. For example, it might be better to call it ButtonContainer.

Comment on lines +45 to +49
child: Image(
height: MediaQuery.of(context).size.height * 0.1,
width: MediaQuery.of(context).size.width * 0.8,
image: AssetImage(assetLocation),
fit: BoxFit.fitWidth),
Copy link
Member

@jimmy-lan jimmy-lan Jul 4, 2021

Choose a reason for hiding this comment

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

This was mentioned on another pull request with @shoko-inori. @shoko-inori Did you find a solution to code this button rather than using an image?

}

/// Sign up button for the welcome page, use the raised button.
class WelcomeButtons extends StatelessWidget {
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for your contribution! I'm a bit confused about the naming of this widget. Seems like we are only having one button inside of it, but the name is plural. Would you mind explaining why It's called WelcomeButtons rather than WelcomeButton? 🤔

@jimmy-lan jimmy-lan requested a review from a team July 7, 2021 00:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automerge Automatically merge pull request when it's ready

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants