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

Full restyle #419

Merged
merged 15 commits into from
Nov 1, 2017
Merged

Full restyle #419

merged 15 commits into from
Nov 1, 2017

Conversation

finnhodgkin
Copy link
Contributor

@finnhodgkin finnhodgkin commented Oct 6, 2017

New styling for:

  • Restyles the nav so the shadow only appears once scrolling begins (pure css)
  • Changes white pages to a white nav
  • Changes the portfolio section on the homepage and portfolio page so it's more responsive and the images can no longer escape the bounds of each element
  • Adds padding and raises the font size of the titles on the home page, portfolio page and blog
  • Changes the design of the blog page to be more in line with the original
  • Restyles the home-page to be like the original designs.
  • Adds a new larger shadow to css for elements like the portfolio boxes and the values paper-page-like title element

navbar

#407

@finnhodgkin finnhodgkin added the awaiting-review An issue or pull request that needs to be reviewed label Oct 6, 2017
@dwylbot dwylbot bot added the awaiting-review An issue or pull request that needs to be reviewed label Oct 6, 2017
@finnhodgkin
Copy link
Contributor Author

finnhodgkin commented Oct 6, 2017

There are some major changes to the homepage that don't fit with the rest of the site but could easily be swapped back or applied to the other pages. I thought it would be worth applying them so people could see what it looked like exactly like the original designs.

Mostly the side borders:
selection_086

@finnhodgkin
Copy link
Contributor Author

The be/make image is a bit messy (I accidentally uninstalled Photoshop 😢). If the restyle gets approved I'm happy to put together a nicer version 😊

@dwyl dwyl deleted a comment from dwylbot bot Oct 6, 2017
@dwyl dwyl deleted a comment from dwylbot bot Oct 6, 2017
@ghost
Copy link

ghost commented Oct 9, 2017

@finnhodgkin https://pixlr.com/editor/ is a nice photoshop alternative

Changes look good to me!

@iteles
Copy link
Member

iteles commented Oct 9, 2017

I promise to review this this afternoon!

@iteles iteles added in-review Issue or pull request that is currently being reviewed by the assigned person and removed awaiting-review An issue or pull request that needs to be reviewed labels Oct 9, 2017
Copy link
Member

@iteles iteles left a comment

Choose a reason for hiding this comment

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

@finnhodgkin Thanks for this! Overall it's looking much tighter and more consistent <3

Not all of these are blockers, I find padding and excess (or lack of) whitespace quite stressful so I would rather get those fixed before this is merged in 😊

I have tested by far and away mostly on desktop, I trust @markwilliamfirth has thought 👍

index.html Outdated
<section class="dwyl-bg-mint ph4 pt5 pb4 tc">
<img src="./img/common/mvp.png" alt="We can be/make your MVP" class="center tc pt4"/>
<section class="ph4 pt5 pb4 tc">
<h2 class="pa0 ma0 f1 b dwyl-dark-gray text-shadow-1">We can <span class="relative transparent"><img src="/img/common/be-make.png" class="dwyl-mint be-make dib relative"/>Make</span> your MVP*</h2>
Copy link
Member

Choose a reason for hiding this comment

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

I'm assuming I'm getting this because I'm just opening the page in the browser and it doesn't know where to load the image from?
screen shot 2017-10-09 at 19 03 42

How does this play out in screen readers or SEO impact?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The weird span/image mix is just leftover mess from trying to get #308 working. Now I know this restyle is going ahead I'll make an image for the section.

index.html Outdated
<section class="dwyl-bg-mint ph4 pt5 pb4 tc">
<img src="./img/common/mvp.png" alt="We can be/make your MVP" class="center tc pt4"/>
<section class="ph4 pt5 pb4 tc">
<h2 class="pa0 ma0 f1 b dwyl-dark-gray text-shadow-1">We can <span class="relative transparent"><img src="/img/common/be-make.png" class="dwyl-mint be-make dib relative"/>Make</span> your MVP*</h2>
Copy link
Member

Choose a reason for hiding this comment

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

alt seems to have disappeared from the image here (still in the commented out one below

index.html Outdated
<section class="dwyl-bg-mint ph4 pt5 pb4 tc">
<img src="./img/common/mvp.png" alt="We can be/make your MVP" class="center tc pt4"/>
<section class="ph4 pt5 pb4 tc">
<h2 class="pa0 ma0 f1 b dwyl-dark-gray text-shadow-1">We can <span class="relative transparent"><img src="/img/common/be-make.png" class="dwyl-mint be-make dib relative"/>Make</span> your MVP*</h2>
Copy link
Member

Choose a reason for hiding this comment

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

Haven't been able to fully test this in mobile because of the missing image, but the line height feels a little off, most likely an optical illusion because of the combination of the all lower case your with no ascenders with the line above and the descender in your with the capitalised MVP in the line below. Just mentioning it for a sanity check though:
screen shot 2017-10-09 at 19 08 04

<li class="v-top dib w-100 w-50-ns f5 dwyl-yellow fw3 mb3 pb2 pr3">Data Visualisation</li>
</ul>
</div>
<a href="#contact" class="dib center link mt2 mb5 ph4 pv2 dwyl-bg-yellow dwyl-dark-gray br2 f5 shadow-4">Enquire Now</a>
Copy link
Member

Choose a reason for hiding this comment

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

The lack of padding at the bottom of this section feels like a bug to me:
screen shot 2017-10-09 at 19 13 15

I know this is what is in the designs (as per the last comment here: #407 (comment)) but the lack of symmetry between the white padding at the top and the abrupt end of this section is completely lost on me.

Would you agree @markwilliamfirth @finnhodgkin ?

Copy link

Choose a reason for hiding this comment

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

These changes are implementing the designs that we accepted, so this question is probably for @harrygfox

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 haven't changed the bottom padding, but the white margins don't exist anymore and I think it makes the bottom look less weird.

index.html Outdated
<section class="dwyl-bg-mint center pv5-ns pb5-ns relative z-0">
</main>

<section class="dwyl-bg-mint center pv5-ns pb5-ns relative z-0 mw9">
Copy link
Member

Choose a reason for hiding this comment

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

Not entirely convinced by this section being boxed in by the sides, I feel like a footer should cover the expanse of the webpage, but not offended enough to ask for the change:
screen shot 2017-10-09 at 19 17 08

It does need to be consistent across the board however (see 404.html) and I think that would be easier if the Training box was enclosed within the white 'Services' box (essentially) and this expanded across the site.

Copy link
Contributor Author

@finnhodgkin finnhodgkin Oct 10, 2017

Choose a reason for hiding this comment

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

I've expanded the footer and the training section:

selection_097

I'm not convinced this looks better, but at least it's more consistent with the rest of the site.

portfolio.html Outdated
<section class="mw9 center ph3">
<h2 class="pl3 pr3 f2 mt0 w-100 tc tl-ns dwyl-dark-gray">Open Source</h2>
<section class="mw8 center ph3">
<h2 class="pl3 pr3 f1 mt0 w-100 tc tl-ns dwyl-dark-gray">Open Source</h2>
<p class="pl3 pr3 f4 b ph0-m dwyl-dark-gray pb3">We also create and maintain over 200 open source repositories on <a href="https://github.com/dwyl" class="dwyl-teal-dark link" >GitHub</a>, check out some of our most popular ones below:</p>
Copy link
Member

Choose a reason for hiding this comment

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

This is a very random use of the dark-teal colour:
screen shot 2017-10-09 at 19 34 52

and searchable!</p>
<img src="./img/common/science-museum-logo.jpg" alt="Science Museum logo" class="w-70 w-75-l pt3 pt2-l"/>
</div>
<div class="dt-m dt-l dt--fixed-m dt--fixed-l mb3 ph3 ph0-ns" style="border-spacing: 1rem 0px;">
Copy link
Member

Choose a reason for hiding this comment

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

These new cards are looking great 🎉

Shall we open an issue about the weird lack of consistency with the github cards directly below it?

team.html Outdated
<section class="dwyl-dark-gray pt5 pb3 pb3-ns ph1 ph0-m ph3-l center tc mw9">
<h1 class="pt4-ns b">Meet the dwylers</h1>
<h1 class="pt4-ns pt6-l b f1 mt0">Meet the dwylers</h1>
Copy link
Member

Choose a reason for hiding this comment

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

That padding though 🙈

It would look better a little more like this:
screen shot 2017-10-09 at 19 37 32

</a>
<a href="/team" class="white link">
<button class="bn dib link ml2 mb3 ph3 pv2 dwyl-bg-mint white br2 f5 shadow-4 pointer istok-web">Meet the Team</button>
<button class="bn dib link mb3 ph3 pv2 dwyl-bg-mint white br2 f5 shadow-4 pointer istok-web">Meet the Team</button>
Copy link
Member

Choose a reason for hiding this comment

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

There seems to be less padding between the 'More than Just Software' section and the 'We believe in' section than between the value themselves:
screen shot 2017-10-09 at 19 40 13

This was quite confusing to the eye when I first scrolled down as I wasn't sure where to focus and this looked like one big glob of text.

values.html Outdated
@@ -116,7 +118,7 @@ <h3 class="dwyl-yellow mt1">People 🤝</h3>
<a href="https://www.focushub.co.uk/" class="dwyl-yellow">a home for our London-based team at Focus Hub.</a>
</p>
</div>
<div class="dib w-45-ns ml4-m ml5-ns v-top">
<div class="dib w-45-ns ml0-m ml5-ns v-top pb5-ns pb0">
Copy link
Member

Choose a reason for hiding this comment

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

Comment on pb above 👍

@iteles iteles removed the in-review Issue or pull request that is currently being reviewed by the assigned person label Oct 9, 2017
@iteles iteles assigned ZooeyMiller and finnhodgkin and unassigned iteles and ZooeyMiller Oct 9, 2017
@finnhodgkin finnhodgkin added in-progress An issue or pull request that is being worked on by the assigned person T1h Time Estimate 1 Hour labels Oct 10, 2017
@finnhodgkin finnhodgkin added T4h Time Estimate 4 Hours and removed T1h Time Estimate 1 Hour labels Oct 10, 2017
@finnhodgkin finnhodgkin assigned ghost and unassigned finnhodgkin Oct 10, 2017
@finnhodgkin finnhodgkin added awaiting-review An issue or pull request that needs to be reviewed and removed in-progress An issue or pull request that is being worked on by the assigned person labels Oct 10, 2017
@iteles iteles added in-review Issue or pull request that is currently being reviewed by the assigned person and removed awaiting-review An issue or pull request that needs to be reviewed labels Nov 1, 2017
Copy link
Member

@iteles iteles 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 all the changes @finnhodgkin !
I think we've left it long enough that we can say for certain that no one has any other comments. Merging 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in-review Issue or pull request that is currently being reviewed by the assigned person T4h Time Estimate 4 Hours
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants