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

implementing new footer #197

Merged
merged 1 commit into from
Jul 29, 2019
Merged

implementing new footer #197

merged 1 commit into from
Jul 29, 2019

Conversation

mansona
Copy link
Member

@mansona mansona commented Jul 25, 2019

Desktop:
Screenshot 2019-07-29 at 21 15 55

Mobile:
Screenshot 2019-07-29 at 21 15 43

The mobile will most likely need to be updated once the mobile designs are finished but it is functional enough for now 👍 (I think anyway 😂)

@mansona mansona requested a review from a team July 25, 2019 19:34
Copy link
Member

@MelSumner MelSumner left a comment

Choose a reason for hiding this comment

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

Thank you for working on this- would you please include screenshots of different viewports with this PR? Thank you!

addon/components/es-footer/es-contributions.js Outdated Show resolved Hide resolved
{{svg-jar link.class}}
{{#each args.socialLinks as |link|}}
<a href={{link.href}} title={{link.title}} aria-label={{link.label}} class="icon-wrapper margin-xsmall small">
{{svg-jar link.class class="icon" }}
Copy link
Member

Choose a reason for hiding this comment

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

why aren't we using fontawesome icons here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm pretty sure we've already discussed this a few times but the new branch for the redesign doesn't have font-awesome as a dependency and therefore doesn't use those icons.

Even regardless of this fact, this is no different to what is already being used in the current master branch: https://github.com/ember-learn/ember-styleguide/blob/master/addon/templates/components/es-footer/es-contributions.hbs#L9

Copy link
Member

Choose a reason for hiding this comment

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

oh interesting. thanks for reminding me.

addon/templates/components/es-footer/es-statement.hbs Outdated Show resolved Hide resolved
@@ -0,0 +1,15 @@
<?xml version="1.0" encoding="UTF-8"?>
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 we already have an existing Ember logo? Why are we replacing it?

Copy link
Member Author

Choose a reason for hiding this comment

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

so this isn't replacing the existing logo, it's adding a new one. The existing logo is the white variation and this one (in ember orange) is being used for the footer.

<?xml version="1.0" encoding="UTF-8"?>
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we should review for all of the .svg logos- I thought we already had these, and would want to know why we're adding different ones.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is just replacing the existing one that was being used in the footer with the official svg logo with colour (because that is what is in the new design)

Copy link
Member

Choose a reason for hiding this comment

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

ahh, I see. I'm not really a fan of multi-color logos in the footer, what do you think about it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was just going with what the design shows but to be honest I quite like it. It makes the footer pop a little bit more 😄 I edited the PR with the screenshots you requested and I think they look great 🎉 (not biased at all 😂)

Copy link
Member

Choose a reason for hiding this comment

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

yes, I also think it looks great, but I wonder if it's a good design decision. It's not the main focus and we don't really want to be drawing anyone's eye specifically here, not more than anything else, anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it a blocker to this PR being merged? I thought the point was to make things like the design and then iterate from there 🤔

@MelSumner
Copy link
Member

Where can we add a TODO for the mobile styles? It should have a "stack the boxes" approach at the very least.

@mansona
Copy link
Member Author

mansona commented Jul 29, 2019

@MelSumner all todos should be recorded in https://github.com/orgs/ember-learn/projects/16 because that is the active working board for the project. Anything that is recorded in there will be addressed.

I will say though that @pichfl is currently working on the sketch file and producing designs for the mobile view currently. If you want I can ask him to throw together a quick GitHub issue and drop it in the project if you would like it tracked?

@MelSumner
Copy link
Member

@mansona that sounds like a good idea, because ideally we'd have a mobile-first approach to the website for the CSS.

For example, if I wanted one font size if I was on a mobile device, then larger if I had a bigger screen, I'd write it like this:

.class-of-element {
  font-size: 1em;

  @media screen and (min-width: 992px) {
    font-size: 1.5em;
  }
}

In my experience, it's a better workflow to know up front what is going to happen, or else you get stuck with really terrible hacks, like media queries that go from desktop to mobile, or worse, like an entirely separate site.

@mansona
Copy link
Member Author

mansona commented Jul 29, 2019

@MelSumner ok, that sounds reasonable 🤔 I think... (I'm not an expert on these things)

I'm confused though, do you see this as a blocker to this PR? I can raise it with @ghislaineguerin and @pichfl because it sounds like it's more of a fundamental concern than I was trying to tackle here 🙈

@MelSumner
Copy link
Member

MelSumner commented Jul 29, 2019

I don't want to prevent shipping this if we have a todo item in the project- want to add one and link it here?

I do think we should prevent shipping additional things if they don't include a basic approach to mobile-first CSS. As a MVP, we should at least:

  • "stack the boxes"- make elements full-width (as is sensible)
  • make paddings and margins smaller (again, as is sensible)
  • double check the font sizes (sometimes a smaller heading font-size is better)

The added bonus of making sure we do this is setting up the media queries now that we'll definitely need later, which makes our later work less of a chore.

@mansona
Copy link
Member Author

mansona commented Jul 29, 2019

@MelSumner how about this: #198 ?

I know it's currently a placeholder but I will coordinate with @pichfl and @ghislaineguerin and get it filled out this week 👍

Copy link
Member

@MelSumner MelSumner left a comment

Choose a reason for hiding this comment

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

Thank you!

@MelSumner MelSumner merged commit ff1db39 into website-redesign-rfc Jul 29, 2019
@delete-merged-branch delete-merged-branch bot deleted the new-footer branch July 29, 2019 20:57
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