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

Deleted HelloWorld, Metamask login. Moved Metamask button to fit in c… #1

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jawndiego
Copy link
Collaborator

deleted metamask component, and hello world componet.

combined both of 'em to fit within the header.vue component.

added some bootstrap packages for sticky-header.

logo added.

fonts added.

…ontainer that contains the logo, the branded text
Copy link
Owner

@tangerine-orange tangerine-orange left a comment

Choose a reason for hiding this comment

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

In general looks good. Not sure if I agree with the decision to use bootstrap or not though.

@@ -3,6 +3,8 @@
## Project setup
```
npm install
or dont
Copy link
Owner

Choose a reason for hiding this comment

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

revert this change

@@ -0,0 +1,6 @@
<template>
Copy link
Owner

Choose a reason for hiding this comment

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

Do we need this file now? If not, delete for now.

@@ -1,25 +1,32 @@
<template>
<img alt="Vue logo" src="./assets/logo.png">
<HelloWorld msg="Welcome to Your Vue.js App"/>
Copy link
Owner

Choose a reason for hiding this comment

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

Let's leave HelloWorld in here for now as the landing page. That way, we can see how the Header acts on a page, instead of just seeing it in isolation. For example, if you do this:

`

` you'll see that the header is transparent, so its content interferes with the main page content.


<b-navbar-button class="col">
<li>{{ msg }}</li>
<button v-on:click="greet" class="enableEthereumButton">Connect</button>
Copy link
Owner

Choose a reason for hiding this comment

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

At some point we'll probably want to pull this button (and the greet function) out into its own component. For now we can leave it like you have it.

"core-js": "^3.6.5",
"vue": "^3.0.0",
"vue-fixed-header": "^3.2.15",
Copy link
Owner

Choose a reason for hiding this comment

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

It seems like you're not using this, but using the bootstrap header instead? If thats true, remove this package.

@@ -8,8 +8,11 @@
"lint": "vue-cli-service lint"
},
"dependencies": {
"@popperjs/core": "^2.9.2",
Copy link
Owner

Choose a reason for hiding this comment

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

What's this for? Are we actually using it in this diff?

@@ -8,8 +8,11 @@
"lint": "vue-cli-service lint"
},
"dependencies": {
"@popperjs/core": "^2.9.2",
"bootstrap": "^5.0.0",
Copy link
Owner

Choose a reason for hiding this comment

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

Since we are downloading bootstrap from the CDN in public/index.html, we don't need to add it as a package here. Let's make a ticket now for converting from CDN usage to npm package usage later on.

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