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

Implement footer block #248

Merged
merged 7 commits into from
May 3, 2017
Merged

Implement footer block #248

merged 7 commits into from
May 3, 2017

Conversation

sophieH29
Copy link
Contributor

No description provided.

"url": "https://gdg.us11.list-manage.com/subscribe/post?u=b7e853a79164ddfdbda3ed77b&id=7993e39fbe",
"name": "b_b7e853a79164ddfdbda3ed77b_7993e39fbe"
},
"gdgLvivUrl": "http://lviv.gdg.org.ua/",
Copy link
Member

Choose a reason for hiding this comment

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

It should not be hardcoded to GDG Lviv. Try something like gdgChapterUrl or extend organizer object:

"organizer": {
    "name": "GDG Lviv",
    "twitter": "gdglviv",
    "url": "..."
  }

"name": "b_b7e853a79164ddfdbda3ed77b_7993e39fbe"
},
"gdgLvivUrl": "http://lviv.gdg.org.ua/",
"codUrl": "https://devfest.gdg.org.ua/cod/",
Copy link
Member

Choose a reason for hiding this comment

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

Should be as part of pages

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know, will be improved since we haven't this page implemented yet

Copy link
Member

Choose a reason for hiding this comment

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

in this case we don't need this info

"url": "https://www.youtube.com/playlist?list=PLt8lEzcLNl31AouwDkbLbLARlQFwNS2en"
}
],
"gplusShareLink": "https://plus.google.com/share?url=https://devfest.gdg.org.ua/",
Copy link
Member

Choose a reason for hiding this comment

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

Why only G+ no one is using it right now :)
Create a generic behavior that includes functions to share with all social networks and is extendable. See https://github.com/gdg-x/hoverboard/blob/master/src/behaviors/share-behavior.html
Also, don't forget about new features that would be nice to include https://developers.google.com/web/updates/2016/10/navigator-share

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's create separate story on this

}
],
"gplusShareLink": "https://plus.google.com/share?url=https://devfest.gdg.org.ua/",
"blogLink": "https://medium.com/gdg-lviv",
Copy link
Member

Choose a reason for hiding this comment

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

Move to organizer object

},
"gdgLvivUrl": "http://lviv.gdg.org.ua/",
"codUrl": "https://devfest.gdg.org.ua/cod/",
"gdgName": "GDG Lviv"
Copy link
Member

Choose a reason for hiding this comment

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

It's organizer + it's not a generic solution. Don't limit application only to GDGs

"followOur": "Follow our",
"followUs": "Follow us",
"emailUs": "Email us",
"blog": "blog",
Copy link
Member

Choose a reason for hiding this comment

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

Lowercase?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

does not matter, since it styled with css to be uppercase

Copy link
Member

Choose a reason for hiding this comment

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

consistency

"eventResources": "Event Resources",
"footerRelBlock": [
{
"headTitle": "About",
Copy link
Member

Choose a reason for hiding this comment

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

maybe just title, it's not a head

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in meaning of that element - yes, it's head - head of column. But ok, can be title

"blog": "blog",
"partnershipResources": "Partnership Resources",
"eventResources": "Event Resources",
"footerRelBlock": [
Copy link
Member

Choose a reason for hiding this comment

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

What means rel ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

relations - there is footer-rel.html element

Copy link
Member

Choose a reason for hiding this comment

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

Maybe just footerBlocks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, cause it's related to footer-rel.html element. Can be footerRelationsBlock, but I would leave it as it is.

Copy link
Member

Choose a reason for hiding this comment

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

I would rather rename footer-rel.html file. It confuses me :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

замахав)


static get is() { return 'footer-block' }

constructor() {
Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary

padding: 0;
}

.gdg-logo {
Copy link
Member

Choose a reason for hiding this comment

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

Don't limit to GDGs

}

a {
-moz-osx-font-smoothing: grayscale;
Copy link
Member

Choose a reason for hiding this comment

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

Move to global styles


static get is() { return 'footer-nav' }

constructor() {
Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary

color: #616161;
padding-bottom: 2px;
text-decoration: none;
transition: border-color .3s ease-out;
Copy link
Member

Choose a reason for hiding this comment

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

Can we move animation global config to variables to make it the same everywhere?


</style>
{% for footerRel in footerRelBlock %}
<div class="col" layout vertical flex wrap flex-auto>
Copy link
Member

@ozasadnyy ozasadnyy Mar 19, 2017

Choose a reason for hiding this comment

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

We don't have any definition of .col. Do we need both flex and flex-auto attributes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what do you mean there is no definition of col?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

flex-auto is enough, will remove flex


static get is() { return 'footer-rel' }

constructor() {
Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary

:host {
padding-left: 4px;
position: relative;
color: var(--footer-text-color);;
Copy link
Member

Choose a reason for hiding this comment

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

Extra semicolon

}

.share-gplus paper-icon-button {
color: var(--red-gplus-color);
Copy link
Member

Choose a reason for hiding this comment

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

Just gplus-color. Because if they change color we don't need to update variable name.

}

.blog h4 {
padding-right: 55px;
Copy link
Member

Choose a reason for hiding this comment

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

Magic number?

}

.fab paper-fab {
transform: scale(1);
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this?

transform: scale(1);
background: #fff;
color: inherit;
transition: transform 200ms cubic-bezier(0, 0, 0.2, 1), -webkit-transform 200ms cubic-bezier(0, 0, 0.2, 1);
Copy link
Member

Choose a reason for hiding this comment

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

A new animation, can we use the previous one?
Write in a canonical way, we are going to use autoprefixer then

transition: transform 200ms cubic-bezier(0, 0, 0.2, 1), -webkit-transform 200ms cubic-bezier(0, 0, 0.2, 1);
will-change: transform;
pointer-events: all;
box-shadow: 0px 0px 8px 0px rgba(0,0,0,0.12), 0px 8px 8px 0px rgba(0,0,0,0.24);
Copy link
Member

Choose a reason for hiding this comment

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

just 0

}

.fab {
transform: translateY(-130px);
Copy link
Member

Choose a reason for hiding this comment

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

If we add position: relative to :host only after min-width: 812px, we don't need to specify this value + if previous block is taller, this number will not work

}

.social-group.blog,
.social-group.social-networks {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need such specific selector? Maybe just .blog, .social-networks?

backToTop(e) {
var scrollDuration = 500;
var scrollStep = -window.scrollY / (scrollDuration / 15),
scrollInterval = setInterval(function(){
Copy link
Member

Choose a reason for hiding this comment

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

setInterval is not good for performance use requestAnimationFrame instead. Some info:
https://hacks.mozilla.org/2011/08/animating-with-javascript-from-setinterval-to-requestanimationframe/


static get is() { return 'mailchimp-subscribe' }

constructor() {
Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

@@ -44,6 +48,10 @@
text-decoration: none;
}

.roboto-font {
Copy link
Member

Choose a reason for hiding this comment

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

@@ -34,7 +35,7 @@
:host {
display: block;
position: relative;
min-height: 100vh;
min-height: 200vh;
Copy link
Member

Choose a reason for hiding this comment

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

2 times viewport height? Why do we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's temporary since there is no any content's static height. As footer has, it will take mostly all space for now.

Copy link
Member

Choose a reason for hiding this comment

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

We can add such values to test on local environments but never push to the repository

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 :)

Copy link
Member

@ozasadnyy ozasadnyy left a comment

Choose a reason for hiding this comment

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

See comments above

"url": "https://www.youtube.com/playlist?list=PLt8lEzcLNl31AouwDkbLbLARlQFwNS2en"
}
],
"share" : [
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to keep this values here? Most probably users will not change them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably yes, but better to have one point to configure the conference website for your needs, then looking for places to change in code

"name": "b_b7e853a79164ddfdbda3ed77b_7993e39fbe"
},
"codUrl": "https://devfest.gdg.org.ua/cod/",
"chapterName": "GDG Lviv"
Copy link
Member

Choose a reason for hiding this comment

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

Can we use organizer.name instead?

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

"footerRelBlock": [
{
"title": "About",
"links": [
Copy link
Member

Choose a reason for hiding this comment

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

All these links will be opened in a new tab? can we configure this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what is a goal of configuring that? I put target="_blank" for each link.


:host {
display: block;
position: absolute;
Copy link
Member

Choose a reason for hiding this comment

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

Why position absolute? this block should be at the bottom

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was googling on different cases, and this is how people usually do that :) if you want the element to put at the bottom, you make position absolute + bottom: 0;

left: 0;
right: 0;
margin: 20px 0 0 0;
box-sizing: border-box;
Copy link
Member

Choose a reason for hiding this comment

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

This behavior should default for all elements, move to shared-styles.

* {
 box-sizing: border-box;
}

<paper-fab class="back-to-top" icon="hoverboard:up" on-click="backToTop"></paper-fab>
</div>
<footer-social layout flex flex-auto horizontal wrap></footer-social>
<footer-rel layout flex horizontal wrap></footer-rel>
Copy link
Member

Choose a reason for hiding this comment

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

What -rel stands for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

footer-relations, the same as footer-nav is navigation

text-decoration: underline;
}

@media (max-width: 811px) {
Copy link
Member

Choose a reason for hiding this comment

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

We have mobile first development. So @media should be used with (min-width: ...)

<a href="{$ organizer.url $}" target="_blank" rel="noopener noreferrer">
<iron-image
class="footer-logo"
src="../../images/gdg-logo.svg"
Copy link
Member

Choose a reason for hiding this comment

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

rename to organizer-logo.svg, please

@@ -42,6 +48,10 @@
a {
color: var(--default-primary-color);
text-decoration: none;
-moz-osx-font-smoothing: grayscale;
Copy link
Member

Choose a reason for hiding this comment

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

Move to global styles * { ... } in shared-styles

};
})();

const AnimationFrame = subclass => class extends subclass {
Copy link
Member

Choose a reason for hiding this comment

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

Class name does not describe the content. Maybe would be better ScrollFunctions or something like that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initially, I wanted to have one mixin with all functions requesting animation frame

let time = Math.max(.1, Math.min(Math.abs(scrollY - scrollTargetY) / speed, .8));

// easing equations from https://github.com/danro/easing-js/blob/master/easing.js
let PI_D2 = Math.PI / 2,
Copy link
Member

Choose a reason for hiding this comment

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

We don't use this variable

@@ -0,0 +1,64 @@
<script>
window.requestAnimFrame = (function(){
Copy link
Member

Choose a reason for hiding this comment

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

We can use standard - window.requestAnimationFrame
http://caniuse.com/#search=requestAnimationFrame

"name": "b_b7e853a79164ddfdbda3ed77b_7993e39fbe"
},
"codUrl": "https://devfest.gdg.org.ua/cod/",
"organizerName": "GDG Lviv"
Copy link
Member

Choose a reason for hiding this comment

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

duplicate

"url": "https://gdg.us11.list-manage.com/subscribe/post?u=b7e853a79164ddfdbda3ed77b&amp;id=7993e39fbe",
"name": "b_b7e853a79164ddfdbda3ed77b_7993e39fbe"
},
"codUrl": "https://devfest.gdg.org.ua/cod/",
Copy link
Member

Choose a reason for hiding this comment

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

remove

@@ -3,5 +3,77 @@
"description": "The biggest Google tech conference in Ukraine carefully crafted for you by GDG community! All about Android, Web and Cloud from the world experts",
"keywords": "event, gdg, gde, devfest, google, programming, android, chrome, polymer, developers, web, cloud, androiddev",
"dates": "October 13-14, 2017",
"locationAddressShort": "Lviv, Ukraine"
"locationAddressShort": "Lviv, Ukraine",
Copy link
Member

Choose a reason for hiding this comment

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

move to locations object

background: var(--footer-background-color);
font-size: 14px;
line-height: 1.5;
text-rendering: optimizeLegibility;
Copy link
Member

Choose a reason for hiding this comment

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

Move to global styles for body

<g id="up">
<path d="M7.41,15.41L12,10.83L16.59,15.41L18,14L12,8L6,14L7.41,15.41Z"></path>
</g>
<g id="gplus-share">
Copy link
Member

Choose a reason for hiding this comment

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

let's be more generic and name such icons -alt instead of -share

Copy link
Member

@ozasadnyy ozasadnyy left a comment

Choose a reason for hiding this comment

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

Good job 👍

@sophieH29 sophieH29 merged commit 113af9d into hoverboard-v2 May 3, 2017
@sophieH29 sophieH29 deleted the footer-v2 branch June 6, 2017 16:42
CmoaToto pushed a commit to paug/android-makers-2019 that referenced this pull request Dec 24, 2018
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