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.

Sign up for GitHub

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

Jump to bottom

Remove contributors list from template, fix template markup and update README files #403

Merged
merged 2 commits into from
Jan 8, 2018
Merged

Remove contributors list from template, fix template markup and update README files #403

merged 2 commits into from
Jan 8, 2018

Conversation

kr8n3r
Copy link

@kr8n3r kr8n3r commented Dec 22, 2017 •
edited
Loading

Contributors list is already out of date. We will need to explore a different way of crediting contributions.

This PR:

  • removes the list from the component view template
  • updates template so no wrapper divs are present in the generated markdown files
  • updates pre and code blocks in generated markdown files
  • updates all the README files with latest changes

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-403 December 22, 2017 15:07 Inactive
@kr8n3r kr8n3r changed the title Remove contributors list Remove contributors list from template and update README files Dec 22, 2017
36degrees
36degrees reviewed Dec 22, 2017
View reviewed changes
Copy link
Contributor

@36degrees 36degrees left a comment

Choose a reason for hiding this comment

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

Some unnecessary bits of HTML being included when the README is transpiled to markdown?

src/components/back-link/README.md Outdated
@@ -1,3 +1,7 @@
<div class="govuk-o-width-container">
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔

src/components/back-link/README.md Outdated
.pipe(sass({
includePaths: 'node_modules/'
}))
<pre> `.pipe(sass({
Copy link
Contributor

Choose a reason for hiding this comment

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

Where did these pre tags come from?

@kr8n3r
Copy link
Author

kr8n3r commented Dec 22, 2017

has there been work on the component template?

@36degrees
Copy link
Contributor

Possibly related to 4c01202#diff-841b0a984418ab47f365fb13d1a0ac89?

@kr8n3r
Copy link
Author

kr8n3r commented Dec 22, 2017

ok will fix that first then and check markdown compiler

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-403 December 22, 2017 18:11 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-403 December 22, 2017 18:27 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-403 December 22, 2017 18:29 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-403 December 22, 2017 18:30 Inactive
@kr8n3r kr8n3r changed the title Remove contributors list from template and update README files Remove contributors list from template, update template files and update README files Dec 22, 2017
@kr8n3r kr8n3r changed the title Remove contributors list from template, update template files and update README files Remove contributors list from template, update template markup and update README files Dec 22, 2017
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-403 December 23, 2017 21:03 Inactive
@kr8n3r kr8n3r changed the title Remove contributors list from template, update template markup and update README files Remove contributors list from template, fix template markup and update README files Jan 1, 2018
@kr8n3r
Copy link
Author

kr8n3r commented Jan 2, 2018

updated with template fixes

@kr8n3r
Copy link
Author

kr8n3r commented Jan 2, 2018

rebased with #405

36degrees
36degrees suggested changes Jan 2, 2018
View reviewed changes
src/components/breadcrumbs/README.md Outdated
})`
</pre>

## Getting updates
Copy link
Contributor

Choose a reason for hiding this comment

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

All of the READMEs appear to be missing everything from here to the license, despite most of it still being in the template?

@kr8n3r
Copy link
Author

kr8n3r commented Jan 2, 2018

yeah missing </

36degrees
36degrees reviewed Jan 4, 2018
View reviewed changes
src/views/component.njk Outdated
@@ -57,72 +56,59 @@
<p class="govuk-body">{% block componentDependencies %}{% endblock %}</p>

<h2 class="govuk-heading-l">Installation</h2>
<pre><code>npm install --save @govuk-frontend/{{ componentName }}</code></pre>

<code>npm install --save @govuk-frontend/{{ componentName }}</code>
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the logic in <pre><code> vs <code>? Looking at the rendered readmes in GitHub, these no longer render as code blocks but as inline. Is that the desired outcome?

Copy link
Author

Choose a reason for hiding this comment

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

its a single line so doesnt stringly need pre. but can unify it

Copy link
Contributor

Choose a reason for hiding this comment

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

I think if it's a single line but not inline it should still be a code block.

I.e. the difference between this inline code and

this single line code block

Copy link
Author

Choose a reason for hiding this comment

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

ql, updated

36degrees
36degrees reviewed Jan 5, 2018
View reviewed changes
Copy link
Contributor

@36degrees 36degrees 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 except for one change I don't understand, which removes the main container from the homepage.

src/views/index.njk
@@ -1,7 +1,6 @@
{% extends "layout-debug.njk" %}

{% block content %}
<div class="govuk-o-main-wrapper">
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this removed? There is now no space between the top of the page and the heading on the index page.

Copy link
Author

Choose a reason for hiding this comment

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

my bad, moved the other one into the logic but forgot to undelete this one.

igloosi added 2 commits January 5, 2018 17:21
Update template markup and markdown logic
0388cca 
List of contributors is already out of date. Will need to explore a
different way of crediting contributions. To generate correct
markdown files the following has also needed to be updated:
Remove contributors list from view template
Update pre, code blocks and move main-wrapper
Move width-container outside of isReadme condition
Update all README files
dcf844c
36degrees
36degrees approved these changes Jan 8, 2018
View reviewed changes
Copy link
Contributor

@36degrees 36degrees left a comment

Choose a reason for hiding this comment

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

👍

@36degrees 36degrees merged commit 4e5bd16 into master Jan 8, 2018
@36degrees 36degrees deleted the remove-contibuters-list branch January 8, 2018 10:00
This was referenced Jan 10, 2018
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@36degrees 36degrees 36degrees approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kr8n3r @36degrees @govuk-design-system-ci