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

Use upstream form components #671

Merged
merged 3 commits into from
Jan 10, 2018
Merged

Use upstream form components #671

merged 3 commits into from
Jan 10, 2018

Conversation

NickColley
Copy link
Contributor

@NickColley NickColley commented Jan 5, 2018

We moved some form components to the gem this PR removes the application ones and use the gem versions instead.

See https://government-frontend-pr-671.herokuapp.com/examples/service_sign_in/service_sign_in/choose-sign-in for an example of a real page using the new components

See the component guide for moved components: https://government-frontend-pr-671.herokuapp.com/component-guide

@tijmenb tijmenb temporarily deployed to government-frontend-pr-671 January 5, 2018 14:12 Inactive
@@ -4,7 +4,7 @@ cy:
common:
last_updated: "Diweddarwyd diwethaf"
components:
radio:
radio: # TODO: Push this upstream to the govuk_publishing_gem and remove from here
Copy link
Contributor Author

@NickColley NickColley Jan 5, 2018

Choose a reason for hiding this comment

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

See this TODO, I made a mistake forgetting to move the Welsh translation of the Radio component when moving it into the gem.

I'll follow this up with another PR, but this is a good illustration of why co-locating files is so important, this would have been pretty hard to mess up if the files were right next to each other.

Copy link
Contributor

Choose a reason for hiding this comment

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

A script would help mitigate this in the short term.

@NickColley NickColley changed the title WIP: Use gem components for forms Use gem components for form components Jan 5, 2018
@NickColley NickColley changed the title Use gem components for form components Use upstream form components Jan 5, 2018
@NickColley
Copy link
Contributor Author

I'm going to bump this with the fix and clean it up.

@import 'components/*';

// Components from govuk_publishing_components gem
@import 'components/fieldset';
Copy link
Contributor

@fofr fofr Jan 5, 2018

Choose a reason for hiding this comment

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

Using the same namespace is confusing isn't it.
I would have expected govuk_publishing_components/fieldset. Developers unfamiliar with the gem will find it hard to locate the source.

Copy link
Contributor Author

@NickColley NickColley Jan 5, 2018

Choose a reason for hiding this comment

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

or gem_components/ to match static

Yeah very, we could consider moving the components, for now this comment should help somewhat.

cc @andysellick

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, govuk_publishing_components/component-name would be good. Firebreak? :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Namespace problem being looked at in alphagov/govuk_publishing_components#136

@NickColley NickColley temporarily deployed to government-frontend-pr-671 January 5, 2018 15:02 Inactive
@tuzz tuzz force-pushed the use-gem-components-for-forms branch from df1b5b4 to 2817a3c Compare January 10, 2018 11:40
@tuzz tuzz merged commit 4af8667 into master Jan 10, 2018
@tuzz tuzz deleted the use-gem-components-for-forms branch January 10, 2018 15:12
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.

6 participants