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

Combine national downloads #796

Closed
wants to merge 8 commits into from
Closed

Conversation

usr110
Copy link
Member

@usr110 usr110 commented Sep 12, 2019

WIP - please don't merge

Combined national download page, for the landing page. Relevant issue: #785

@usr110 usr110 requested a review from nikolai-b September 12, 2019 11:03
Copy link
Member

@Robinlovelace Robinlovelace left a comment

Choose a reason for hiding this comment

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

Looks good. Not tested, let us know when this is ready for testing.

@@ -106,5 +106,6 @@
</div>
</div>
</div>
</div>
Copy link
Member

Choose a reason for hiding this comment

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

Does this new closing tag have an opening <div>?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, hence I added it

Copy link
Member

Choose a reason for hiding this comment

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

Great. Asking because it seems you added a closing tag without adding an opening tag.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, it was missing. See the comment with the commit Missing closing div tag

Copy link
Member

Choose a reason for hiding this comment

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

Sure, it was missing. See the comment with the commit Missing closing div tag

Aha I missed that. Good fix.

@usr110
Copy link
Member Author

usr110 commented Sep 12, 2019

It is ready for testing @Robinlovelace

Open this file: pct-shiny/regions_www/tabs/download_national.html, to see how it looks like.

@nikolai-b you might have some ideas how to introduce subsections - or perhaps a divider (like a solid line to differentiate commute from school data)

Copy link
Member

@Robinlovelace Robinlovelace left a comment

Choose a reason for hiding this comment

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

Looks fine, would like to see on the test server before suggesting any changes, but suggest we sort the urgent issues first.

@nikolai-b
Copy link
Contributor

Sorry for the slow review! The pct-shiny/regions_www/tabs/download_national.html needs to be a full HTML file starting with

<!DOCTYPE html>
<html xmlns="https://www.w3.org/1999/xhtml">

and also including the navigation. I can update it if you like?

@usr110
Copy link
Member Author

usr110 commented Oct 2, 2019

No worries and thanks @nikolai-b

Re missing HTML tags, I've added them here: 9834170

Hope it's all fine now.

@nikolai-b
Copy link
Contributor

When I view the page it looks like this:

Screenshot-2019-10-13 The Propensity to Cycle Tool - National Data(1)

Is that what you intended?

Sorry there isn't an easy way for us to develop it locally yet 😢 . I've pushed it to the staging server so you can see and edit (but any merges will remove it)

@Robinlovelace Robinlovelace marked this pull request as draft August 6, 2020 16:59
@Robinlovelace
Copy link
Member

Is this PR still alive @nikolai-b and @usr110 ? I've converted it to a draft having seen the latest comment from Nikolai.

Many thanks Ali, looks like it's an improvement overall, will be great to get this out there.

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.

3 participants