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

Render HTML publication format #111

Merged
merged 11 commits into from Feb 29, 2016
Merged

Render HTML publication format #111

merged 11 commits into from Feb 29, 2016

Conversation

@fofr
Copy link
Contributor

@fofr fofr commented Feb 24, 2016

This is an all the basics PR, it doesn't include the following features:

  • a contents link that moves as you scroll
  • organisation logos
  • working fractions (if changes are necessary)

Does include:

  • Rendering of HTML publication content using the new govspeak-html-publication component. Depends on alphagov/static#744
  • Rendering of the blue HTML publication header, with a contents list that is provided by the content item
  • Add files for styles, template, presenter and tests
  • Use HtmlPublication rather than HTMLPublication as that’s what Rails expects.

Tests also depend on alphagov/govuk-content-schemas#250 (hence the test fails)

https://trello.com/c/cJGrtnc1/286-5-html-publications-migration-front-end-work-large

html-pub-format

Add files for styles, template, presenter and tests

Use `HtmlPublication` rather than `HTMLPublication` as that’s what
Rails expects.
@benlovell
benlovell reviewed Feb 24, 2016
View changes
app/presenters/html_publication_presenter.rb Outdated
end

def format_sub_type
content_item["links"]["parent"][0]["format_sub_type"]

This comment has been minimized.

@benlovell

benlovell Feb 24, 2016
Contributor

Maybe a private helper method for parent that does the reaching in could simplify this?

@benlovell
benlovell reviewed Feb 24, 2016
View changes
app/presenters/html_publication_presenter.rb Outdated
end

def parent_base_path
content_item["links"]["parent"][0]["base_path"]

This comment has been minimized.

@benlovell

benlovell Feb 24, 2016
Contributor

Said helper method could be used here also.

@dsingleton
Copy link
Contributor

@dsingleton dsingleton commented Feb 25, 2016

I'm picking up this work from @fofr while he's away, it's already 95% there.

After the changes in the schema/examples fractions now Just Work™, as the SCSS already existed in the Govspeak Component. Though there isn't a component fixture for fractions, so i'll add one and double check print behaves as we expect. (Edit, done: alphagov/static#747)

I've added the helper @benlovell suggested, which reduces some duplication 👍 and updated the schema to avoid some weird HTML whitespace issues, which @benlovell is looking at more deeply

I'm happy with leaving org-logos and the fixed contents link to follow up PRs.

I want to have a look at the integration testing, as it's only covering one of the schema examples and I want to make sure we're not missing any cases. After that I think this is 🚢-able.

@dsingleton
dsingleton reviewed Feb 25, 2016
View changes
app/assets/stylesheets/mixins/_white-links.scss Outdated
@@ -0,0 +1,30 @@
// TODO (@bfirsh): refactor frontend_toolkit external link mixin to work with

This comment has been minimized.

@dsingleton

dsingleton Feb 25, 2016
Contributor

This is in a few places now, is it viable to move into FET?

This comment has been minimized.

@fofr

fofr Feb 29, 2016
Author Contributor

Follow up PR needed I think.

This comment has been minimized.

@dsingleton

dsingleton Feb 29, 2016
Contributor

👍

This comment has been minimized.

@fofr

fofr Feb 29, 2016
Author Contributor

White external links aren't needed for HTML publications, they were added for consultations. Removing this and the images from this PR.

@dsingleton
dsingleton reviewed Feb 25, 2016
View changes
app/assets/stylesheets/views/_html-publication.scss Outdated
@@ -0,0 +1,84 @@
.html-publication {

.govuk-title .context {

This comment has been minimized.

@dsingleton

dsingleton Feb 25, 2016
Contributor

I don't think we should be relying on the internal markup of a component, that can change independently of this application.

The risk of this approach is that changes to the component, in markup/selectors, or explicitly setting colours on elements that don't have them could break this page without static being aware of the dependency. And it's generally a pattern we try and avoid.

This feels more like a mode of the component and the responsibility for a white links/different margin version should live within the component, not the app.

Alternatively, we treat it as a non standard page title, and don't use the component at all, setting all the styles locally.

@fofr fofr changed the title [DO NOT MERGE] Render HTML publication format Render HTML publication format Feb 29, 2016
@fofr
Copy link
Contributor Author

@fofr fofr commented Feb 29, 2016

@dsingleton Title component changes made in bd99c4f

fofr and others added 10 commits Feb 22, 2016
* Port directly from Whitehall
* Don't port white external links mixin, contents lists in HTML pubs
can't contain external links.

https://github.com/alphagov/whitehall/blob/590fee63149416d4167b1151a684e
50cd2b162ee/app/assets/stylesheets/frontend/styleguide/_typography.scss#
L126
Allow nested components to work.
Use a `simple_header` flag (that defaults to true) to decide whether to
hide or show the proposition header.
`display_time` provides a generic date format that should be re-used
between presenters.
* Use govspeak html publication component
* Port blue heading styles from Whitehall
These overrides should be moved to either:
* a separate component
* a mode on the existing component

eg. if the `.context` class changed name then the style overrides would
no longer work.
* Doesn’t yet handle the positioning to top right of the page
* Port Estonian translation
Prevents line length from going too wide
The HTML publication title is similar but not the same. Avoid brittle
overriding of styles and component modes by porting the markup and
styles for the relatively simple pattern.

* Port responsive margin mixins from static, these, if deemed correct
and easily re-usable, can eventually be moved to front-end toolkit.
@fofr fofr force-pushed the html-publications branch to 488e73e Feb 29, 2016
dsingleton added a commit that referenced this pull request Feb 29, 2016
Render HTML publication format
@dsingleton dsingleton merged commit 60954ab into master Feb 29, 2016
1 check passed
1 check passed
default Build #503 succeeded on Jenkins
Details
@dsingleton dsingleton deleted the html-publications branch Feb 29, 2016
@dsingleton
Copy link
Contributor

@dsingleton dsingleton commented Feb 29, 2016

Decided against adding the additional integration tests for other content examples - they vary that much and where they do tends to be inside markup blocks that are rendered as is, which we're less concerned about testing in this app.

When we add departmental logo support that would be probably deserve an interrogation test for that example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.