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

Closes #13 #25

Open
wants to merge 3 commits into
base: development
Choose a base branch
from
Open

Closes #13 #25

wants to merge 3 commits into from

Conversation

peoray
Copy link

@peoray peoray commented Dec 13, 2018

Created Landing page following the mockup design provided
ref: #13

@ghost ghost force-pushed the feature/create-landing-page branch from 44b4567 to 9ed4ee5 Compare December 14, 2018 20:17
@ghost
Copy link

ghost commented Dec 14, 2018

Rebased the branch to correctly sync with 8a10dad (current development branch) per @peoray's request

@ghost ghost changed the title Feature/create landing page Closes #13 Dec 14, 2018
Copy link
Author

@peoray peoray left a comment

Choose a reason for hiding this comment

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

Looks good to me

@peoray peoray requested review from a user and jidemobell December 20, 2018 10:42
@peoray
Copy link
Author

peoray commented Dec 20, 2018

Review requested

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

  • The landing page doesn't fully implement the design on the task (mobile and tablet design are not present, the icon is not the same and the second button is not in the page)
  • There are extra tags (empty <style /> and/or <script />) as well as extra indentation on Landing.vue's template. I'm surprised eslint doesn't complain about it.
  • Feel free to remove the extra items generated by the scaffolding app. An example would be the logo or the HelloWorld component.
  • App.vue: We'll be using the router to change between pages, thus we need to render <router-view /> inside <v-content /> instead of individual components
  • caf9a87: Commit name should be descriptive. What does it do ?

@peoray
Copy link
Author

peoray commented Jan 15, 2019

Surprised I didn't see this. But a lot of what you are saying I don't understand.

I skipped one of the buttons from the design, as it wasn't the final design and I didn't want to get carried away and opted for one. From my own end, it's very responsive

@peoray
Copy link
Author

peoray commented Jan 15, 2019

As for the router, there is no router at the moment, so I didn't include it anywhere, surprised you said it's in the other files

@peoray
Copy link
Author

peoray commented Jan 15, 2019

As for the empty script and styles, atm, nothing is being added there and it's something we should not worry about.

Initial commit is basically the first commit I did, where I added Vuetify and did a little file structuring and naming

@jidemobell
Copy link

  • I'm not really sure of all happening here. I also noticed the absence of routes and empty script/styles. I'm assuming the card is meant to just generate the Landing page and other work will come later

  • what is still required to close this?

@jidemobell jidemobell closed this Jan 19, 2019
@jidemobell jidemobell reopened this Jan 19, 2019
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.

2 participants