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

Namespace components hosted in gem #136

Merged
merged 5 commits into from Jan 9, 2018
Merged

Namespace components hosted in gem #136

merged 5 commits into from Jan 9, 2018

Conversation

@fofr
Copy link
Contributor

@fofr fofr commented Jan 9, 2018

Move all components hosted by the gem to be within govuk_publishing_components.

Usage in app is now like:

<%= render "govuk_publishing_components/components/radio", … %>
Old New
assets/components/* assets/govuk_publishing_components/components/*
javascripts/components/* javascripts/govuk_publishing_components/components/*
views/components/* views/govuk_publishing_components/components/*

The problem

Before namespacing, in an app you would see something like render "components/blah" which you'd expect to be in app/views/components. When you can't find it there, it's not clear where to find the source.

Equally it led to peculiar includes like:

@import 'components/*';
@import 'components/task-list';
@import 'components/task-list-header';
@import 'components/task-list-related';

If an app and the gem both have a component with the same name, it is not clear which one would have precedence. By setting a namespace it's explicit.

This is a breaking change

fofr added 4 commits Jan 9, 2018
Keep component print styles inline with app convention of placing files
within a subdirectory (see government-frontend).
@fofr fofr force-pushed the namespace-gem-components branch from 807e1a2 to 06815b6 Jan 9, 2018
@fofr fofr requested a review from andysellick Jan 9, 2018
@fofr fofr assigned nickcolley and unassigned nickcolley Jan 9, 2018
@fofr fofr requested a review from nickcolley Jan 9, 2018
* Import mixin from govuk_frontend_toolkit
* This mixin was unavailable in government-frontend when the component
is namespaced
fofr added a commit to alphagov/government-frontend that referenced this pull request Jan 9, 2018
@fofr
Copy link
Contributor Author

@fofr fofr commented Jan 9, 2018

Example of updating government-frontend after this is versioned:
https://github.com/alphagov/government-frontend/compare/example-gem-update

I found that I needed to explicitly import the css3 mixin for box sizing in 607b10b

@nickcolley
Copy link
Contributor

@nickcolley nickcolley commented Jan 9, 2018

When using this namespace practically is it too verbose?

@fofr
Copy link
Contributor Author

@fofr fofr commented Jan 9, 2018

@nickcolley I think the confusion of using a custom namespace would be worse than the problem of length with this one

@fofr
Copy link
Contributor Author

@fofr fofr commented Jan 9, 2018

An alternative to the long namespace would be to have govuk_publishing_components/back_link rather than govuk_publishing_components/components/back_link.

I avoided that in this PR because I thought it would make it unclear what was needed for components and what was needed for the component guide.

Copy link
Contributor

@andysellick andysellick left a comment

More verbose and will mean a lot of changes to apps needed, but sensible.

@nickcolley
Copy link
Contributor

@nickcolley nickcolley commented Jan 9, 2018

I can't block this but I think having such long verbose names will be annoying for people, since it'll make their markup much harder to read.

Edit: We've had big problems with magic in the past, and this change makes govuk_publishing_components a lot less magic than static which is a good change 😄

@nickcolley nickcolley removed their request for review Jan 9, 2018
@fofr fofr merged commit a4f737c into master Jan 9, 2018
1 check passed
1 check passed
continuous-integration/jenkins/branch This commit looks good
Details
@fofr fofr deleted the namespace-gem-components branch Jan 9, 2018
fofr added a commit that referenced this pull request Jan 9, 2018
* Namespace hosted components with `govuk_publishing_components` (PR
#136)
  * References to components hosted in gem need to point to
`govuk_publishing_components/components` rather than `/components`.
This includes stylesheets, partials and javascripts.
  * References to task list print styles must be updated to point at
the new /print subdirectory
This was referenced Jan 9, 2018
fofr added a commit that referenced this pull request Jan 11, 2018
* Combine last two steps when moving
* Remove detail about which app takes precedence and encourage component deletion earlier
sihugh added a commit to alphagov/manuals-frontend that referenced this pull request Jan 16, 2018
[These were moved in govuk_publishing_components](alphagov/govuk_publishing_components#136)
to get them under a gem specific namespace.
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.