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

Back to top arrow style updated #367

Merged
merged 11 commits into from
Jun 12, 2017
Merged

Back to top arrow style updated #367

merged 11 commits into from
Jun 12, 2017

Conversation

andysellick
Copy link
Contributor

@andysellick andysellick commented May 19, 2017

  • SVG arrow added to fit with new design
  • Font size also adjusted to fit new design

Before:

screen shot 2017-05-22 at 08 57 19

After:

screen shot 2017-05-19 at 15 31 28

https://trello.com/c/J6YkyF1T/356-deployment-back-to-top-button-2

Now relates to alphagov/static#1067

* SVG arrow added to fit with new design
* Font size also adjusted to fit new design
@boffbowsh boffbowsh temporarily deployed to government-frontend-pr-367 May 19, 2017 14:31 Inactive
* Moved to using an inline SVG so now aligns with link text colour in all states (e.g. visited, hover)
@boffbowsh boffbowsh temporarily deployed to government-frontend-pr-367 May 19, 2017 15:08 Inactive
data-module="track-click"
href="#contents">
<svg class="icon" width="14" height="17" viewBox="0 0 14 17" version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"><g transform="translate(-885.000000, -1380.000000) translate(886.000000, 1380.000000) translate(0.000000, 1.000000)" stroke-width="2" stroke="currentColor" fill="none" fill-rule="evenodd"><polyline points="12 6.61354167 6.10565074 0.719192411 0.312351484 6.51249167"/><path d="M6,1.52415365 L6,15" stroke-linecap="square"/></g></svg>
Contents
Copy link
Contributor

Choose a reason for hiding this comment

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

This should still be translated. It used to be t('content_item.contents').

Copy link
Contributor

Choose a reason for hiding this comment

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

You could keep the link_to method by doing this:

<%= link_to(params) do %>
 Link text
<% end %>

@fofr
Copy link
Contributor

fofr commented May 22, 2017

Would be good to show a before/after screenshot.

@boffbowsh boffbowsh temporarily deployed to government-frontend-pr-367 May 22, 2017 07:56 Inactive
@@ -0,0 +1 @@
<svg width="14" height="17" viewBox="0 0 14 17" version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"><g transform="translate(-885.000000, -1380.000000) translate(886.000000, 1380.000000) translate(0.000000, 1.000000)" stroke-width="2" stroke="#005EA5" fill="none" fill-rule="evenodd"><polyline points="12 6.61354167 6.10565074 0.719192411 0.312351484 6.51249167"/><path d="M6,1.52415365 L6,15" stroke-linecap="square"/></g></svg>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this file needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, good point

@boffbowsh boffbowsh temporarily deployed to government-frontend-pr-367 May 22, 2017 08:11 Inactive
@fofr
Copy link
Contributor

fofr commented May 22, 2017

Tested this on old IE and it looks fine:
screen shot 2017-05-22 at 09 54 09

Tested on mobile and the arrow looks too big (font-size is reduced):
screen shot 2017-05-22 at 09 54 53

@boffbowsh boffbowsh temporarily deployed to government-frontend-pr-367 May 22, 2017 09:36 Inactive
@fofr
Copy link
Contributor

fofr commented May 22, 2017

@NickColley Can you add updated screenshots?

@NickColley
Copy link
Contributor

@fofr Andy is doing some additional work on this

* Added to document collection template
* Small CSS change to parent wrapper, now position relative to support absolute positioning of contents link when scrolled beyond limit of parent container
@boffbowsh boffbowsh temporarily deployed to government-frontend-pr-367 May 22, 2017 12:41 Inactive
@NickColley
Copy link
Contributor

@fofr
Copy link
Contributor

fofr commented May 22, 2017

I was hoping we could keep the design updates and "adding to new formats" as separate PRs. One shouldn't block the other.

@boffbowsh boffbowsh temporarily deployed to government-frontend-pr-367 May 23, 2017 07:21 Inactive
@NickColley
Copy link
Contributor

image

I think the nesting might be off, since it looks like we're missing the grids gutter padding

* When it occurs within the grid, the grid's negative margin slightly mispositions the back to contents link to the left, CSS added for this specific case to push it back in again.
* One alternative would be to rewrite the grid to not have an outer negative margin, but this would be overkill for this right now.
@boffbowsh boffbowsh temporarily deployed to government-frontend-pr-367 May 23, 2017 12:49 Inactive
@andysellick
Copy link
Contributor Author

Nesting should be fixed now.

@boffbowsh boffbowsh temporarily deployed to government-frontend-pr-367 May 25, 2017 11:14 Inactive
@NickColley
Copy link
Contributor

NickColley commented May 25, 2017

I've noticed for some of the pages the contents link will spill over the end, I wonder if we can address this? It feels really odd at the moment.

I'm able to reproduce this by scrolling quickly on the following page:

screen shot 2017-05-25 at 14 47 14

@boffbowsh boffbowsh temporarily deployed to government-frontend-pr-367 June 5, 2017 12:03 Inactive
@NickColley
Copy link
Contributor

NickColley commented Jun 5, 2017

Checking the live site, the issue I've raised above is already an issue, so we're going to block this on updating static since this adds already existing buggy behaviour to more pages.

@boffbowsh boffbowsh temporarily deployed to government-frontend-pr-367 June 5, 2017 13:06 Inactive
@NickColley NickColley changed the title Back to top arrow style updated [Do not merge] Back to top arrow style updated Jun 5, 2017
@andysellick
Copy link
Contributor Author

Fix added to static... alphagov/static#1067

@@ -42,6 +46,9 @@
</ol>
<% end %>
</div>
<div data-sticky-element class="govuk-sticky-element">
Copy link
Contributor

Choose a reason for hiding this comment

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

Use of the class name govuk-sticky-element hints that it's a component, which it isn't.

@fofr
Copy link
Contributor

fofr commented Jun 7, 2017

How does this look on right to left content?

@andysellick
Copy link
Contributor Author

I can't find a page with right to left content where this is used. Any suggestions?

@andysellick
Copy link
Contributor Author

Seems to be okay on RTL. Have added some more example links to the card.

@andysellick andysellick changed the title [Do not merge] Back to top arrow style updated Back to top arrow style updated Jun 12, 2017
@NickColley NickColley merged commit 0b83bf5 into master Jun 12, 2017
@NickColley NickColley deleted the back-to-top-link branch June 12, 2017 12:48
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.

4 participants