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

Remove README content from review app #482

Merged
merged 5 commits into from
Feb 15, 2018

Conversation

kr8n3r
Copy link

@kr8n3r kr8n3r commented Jan 30, 2018

This PR

  • removes the component description, guidance and table of arguments from
    the component view within the review app. This just leaves the component title and the examples.

  • creates a separate template in app/views for generating README files

  • adds logic to each component's index.njk file to extend the appropriate templates based on the isReadme: true parameter that is we pass to the nunjucks enviroment.

Trello: https://trello.com/c/rUJHVyt3/612-remove-readme-content-from-components-view-in-review-app

Note: this is until we move all content into YAML file and remove component index.njk files

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-482 January 30, 2018 09:42 Inactive
@36degrees
Copy link
Contributor

Would it make more sense at this point to split the template for the README and the template for the component view entirely?

Given the only thing that's common between the two is the title and the examples which are already wrapped up in a macro.

@kr8n3r
Copy link
Author

kr8n3r commented Jan 30, 2018

Updated PR.

@kr8n3r kr8n3r force-pushed the remove-readme-content-view-template branch from ac3fbd8 to e6b4cbb Compare January 30, 2018 14:39
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-482 January 30, 2018 14:40 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-482 January 30, 2018 14:50 Inactive
@kr8n3r kr8n3r force-pushed the remove-readme-content-view-template branch from 7cde775 to 758732b Compare January 30, 2018 16:44
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-482 January 30, 2018 16:44 Inactive
@kr8n3r kr8n3r force-pushed the remove-readme-content-view-template branch from 758732b to cfb9c44 Compare February 2, 2018 14:25
@alex-ju
Copy link
Contributor

alex-ju commented Feb 5, 2018

Discussed with @igloosi and @NickColley. It's not ideal but i think looks good for now. It feels odd that we're passing data both downstream and upstream, which makes things confusing, especially for newcomers. Part of refactoring would be to pass data only downstream, which will simply these views to be inherited from one layout without the need to check for upstream data like isReadme.

@kr8n3r
Copy link
Author

kr8n3r commented Feb 6, 2018

shall we wait for the today's session outcome before doing anything with this?

NickColley
NickColley previously approved these changes Feb 7, 2018
@NickColley NickColley dismissed their stale review February 7, 2018 14:24

I agree, I'm not sure we know how we want this to change yet.

@kr8n3r kr8n3r force-pushed the remove-readme-content-view-template branch from cfb9c44 to 74ae0ed Compare February 8, 2018 15:31
@kr8n3r
Copy link
Author

kr8n3r commented Feb 14, 2018

will need to be reviewed after #528

Jani Kraner added 4 commits February 15, 2018 12:00
Remove the component description, guidance and table of arguments from
the component view within the review app. This will leave just the
component title and the examples.
We've decided to extend the README generation from a separate template
instead of having multiple 'if/else' or 'if not' statements in one
template.
Inside generate:readme task we add a global 'isReadme: true' to the
nunjucks environment that we use to determine which content should go
where.

We've use this to now to extend the correct template in each of the
components 'index.njk' files.
@kr8n3r kr8n3r force-pushed the remove-readme-content-view-template branch from 444adad to 6882ec2 Compare February 15, 2018 12:00
@kr8n3r
Copy link
Author

kr8n3r commented Feb 15, 2018

rebased with latest changes

@kr8n3r kr8n3r merged commit db83297 into master Feb 15, 2018
@kr8n3r kr8n3r deleted the remove-readme-content-view-template branch February 15, 2018 17:17
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.

5 participants