Move form components into gem #116

NickColley · 2018-01-03T15:01:11Z

Moves form related components needed in email-alert-frontend into the gem so they can be used across applications.

We're avoiding static here since these pages need strong integration tests within the applications they are used in, and static mocks components.

These were originally implemented in the following pull requests to government-frontend

We'll move the error related components later to make the PR smaller.

We will do a follow up PR to remove these components from government-frontend

As part of https://trello.com/c/HSre7K10/453-style-the-mvp-email-collection-page-needed-by-jan

tuzz · 2018-01-03T16:06:00Z

startup.sh

+bundle install
+
+PLEK_SERVICE_STATIC_URI=${PLEK_SERVICE_STATIC_URI-assets.publishing.service.gov.uk} \
+bundle exec rackup spec/dummy/config.ru --port=3212 --host=0.0.0.0


We can remove this as running the app is addressed in #115

tuzz · 2018-01-03T16:06:23Z

spec/components/radio_test_spec.rb

+      expect(page).to_not have_selector("[@class='gem-c-radio__input'][@value='govuk-verify'][@checked='checked']", visible: input_visible)
+    end
+  end
+end


Good job on fixing these tests, they look tricky. 🎉

This component was originally added in alphagov/government-frontend#552 Moved from government-frontend for use in email-alert-frontend Also renames the namespace to `gem`

Moved from government-frontend for use in email-alert-frontend Also renames the namespace to `gem` This was originally implemented in government-frontend in alphagov/government-frontend#552

Moved from government-frontend for use in email-alert-frontend Also renames the namespace to `gem` Originally implemented in alphagov/government-frontend#561

fofr · 2018-01-04T10:56:22Z

app/views/components/_label.html.erb

+  classes ||= ''
+  text_classes ||= ''
+  hint_text_classes ||= ''
+  html_for ||= ''


Is for reserved?

Mirroring the DOM here https://developer.mozilla.org/en-US/docs/Web/API/HTMLLabelElement/htmlFor

fofr · 2018-01-04T11:41:46Z

Should the components be called:

form-radio-button
form-label

So their ID and file name matches their component name?

NickColley · 2018-01-04T11:43:42Z

@fofr I believe the components should be called how they're called, but ideally the guide would have categories to group components. I've halfway got there by prefixing the name that is displayed...

Can we raise an issue to allow grouping by category maybe?

fofr · 2018-01-04T13:09:20Z

app/assets/stylesheets/components/helpers/_variables.scss

@@ -0,0 +1,25 @@
+// Forked from GOV.UK Frontend, namespace changed to ensure no conflicts.
+
+$gem-spacing-scale-0: 0;


Would you expect an app in a component to also use these variables? eg Spacing is a common thing.

Hmm, that's a good point.

For now since these are forked lets keep them in here, the main reason to namespace these is to avoid collisions when importing GOV.UK Frontend, I'd imagine long term using the proper GOV.UK Frontend scale variables within downstream components is OK?

Sat next to Chris and he said "Sure"

andysellick

Sorry, apparently was a bit slow in getting this in. Have only reviewed the first commit so far.

andysellick · 2018-01-04T13:39:51Z

app/assets/stylesheets/components/_fieldset.scss

+  // Fix legend text wrapping in Edge and IE
+  // 1. IE9-11 & Edge 12-13
+  // 2. IE8-11
+  box-sizing: border-box; // 1


Isn't there a mixin for this? Is there a reason it's not being used here?

I'm not aware of a mixin for box-sizing

https://github.com/alphagov/govuk_frontend_toolkit/blob/452f82e7538d7f5b8f73ee7d81abea9bc857af13/stylesheets/_css3.scss#L68

I don't think it's useful to use this mixin since the support for the unprefixed version is so high at 98%

https://caniuse.com/#feat=css3-boxsizing

andysellick · 2018-01-04T13:44:37Z

app/views/components/_label.html.erb

@@ -0,0 +1,40 @@
+<%
+  classes ||= ''


Why are most of these declarations empty strings, apart from bold, which is false? They all mostly seem to be handled the same way i.e. if (thing) do something with (thing).

Bold is a boolean that toggles, the others expect strings, so it seemed reasonable to instantiate them with their expected types

Except they're all being treated the same way, as described. Seems inconsistent, but maybe that's just me.

What issues are you anticipating? This is how most of the components are done as far as I know?

andysellick · 2018-01-04T13:48:52Z

app/views/components/_label.html.erb

+        gem-c-label__hint
+        <%= hint_text_classes if hint_text_classes %>
+      "
+      id="<%= hint_id %>"


hint_id looks like an optional parameter but it seems like it's quite important for accessibility? Would it be worth doing a check here to ensure developers remember to pass it?

If hint_text is used but hint_id is not passed the component will throw an exception

Oh, I suppose it will, won't it. Ah okay.

andysellick · 2018-01-04T13:53:16Z

app/views/components/_label.html.erb

+      gem-c-label__text
+      <%= text_classes if text_classes %>
+    "
+    <% if html_for %>


When would a label not have a for attribute?

Addressed in another PR

andysellick · 2018-01-04T13:58:50Z

spec/components/label_test_spec.rb

+    assert_select ".gem-c-label__text", text: "National Insurance number"
+    assert_select ".gem-c-label--bold"
+  end
+end


I don't see a test for some of the parameters (perhaps added in a later commit, will check) i.e. classes, text_classes, hint_text_classes, html_for.

Addressed in another PR, but we havent tested or documented the classes yet, feel free to raise an issue.

andysellick · 2018-01-04T13:59:38Z

app/assets/stylesheets/components/_radio.scss

+  padding: 0 0 0 em(40px, 19px);
+
+  clear: left;
+


Is there a reason for all the line spaces here?

Maybe clean them up at some point?

Will raise an issue to lint the scss in this repo 👍

#124

NickColley force-pushed the add-form-components branch 2 times, most recently from a395080 to c0f7806 Compare January 3, 2018 15:39

tuzz previously requested changes Jan 3, 2018

View reviewed changes

NickColley force-pushed the add-form-components branch from c0f7806 to a11c968 Compare January 3, 2018 16:25

NickColley added 3 commits January 4, 2018 10:47

Add Label component

6e58bf1

This component was originally added in alphagov/government-frontend#552 Moved from government-frontend for use in email-alert-frontend Also renames the namespace to `gem`

Add Radio component

f5da337

Moved from government-frontend for use in email-alert-frontend Also renames the namespace to `gem` This was originally implemented in government-frontend in alphagov/government-frontend#552

Add Fieldset component

7333c6f

Moved from government-frontend for use in email-alert-frontend Also renames the namespace to `gem` Originally implemented in alphagov/government-frontend#561

NickColley force-pushed the add-form-components branch from a11c968 to 7333c6f Compare January 4, 2018 10:49

NickColley changed the title ~~WIP: Move form components into gem~~ Move form components into gem Jan 4, 2018

fofr reviewed Jan 4, 2018

View reviewed changes

NickColley mentioned this pull request Jan 4, 2018

Add a way to group components in a category #120

Closed

fofr reviewed Jan 4, 2018

View reviewed changes

fofr approved these changes Jan 4, 2018

View reviewed changes

NickColley merged commit e59f5fd into master Jan 4, 2018

NickColley deleted the add-form-components branch January 4, 2018 14:17

andysellick reviewed Jan 4, 2018

View reviewed changes

andysellick mentioned this pull request Jan 5, 2018

Bump gem to 3.2.0 #127

Merged

NickColley mentioned this pull request Jan 5, 2018

Use upstream form components alphagov/government-frontend#671

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move form components into gem #116

Move form components into gem #116

NickColley commented Jan 3, 2018 •

edited

Loading

tuzz Jan 3, 2018

tuzz Jan 3, 2018

fofr Jan 4, 2018

NickColley Jan 4, 2018

fofr commented Jan 4, 2018 •

edited

Loading

NickColley commented Jan 4, 2018

fofr Jan 4, 2018

NickColley Jan 4, 2018

andysellick left a comment

andysellick Jan 4, 2018

NickColley Jan 4, 2018

andysellick Jan 4, 2018

NickColley Jan 4, 2018

andysellick Jan 4, 2018

NickColley Jan 4, 2018

andysellick Jan 4, 2018

NickColley Jan 4, 2018

andysellick Jan 4, 2018

NickColley Jan 4, 2018

andysellick Jan 4, 2018

andysellick Jan 4, 2018

NickColley Jan 4, 2018

andysellick Jan 4, 2018

NickColley Jan 4, 2018

andysellick Jan 4, 2018

NickColley Jan 4, 2018

andysellick Jan 4, 2018

NickColley Jan 4, 2018 •

edited

Loading

		@@ -0,0 +1,25 @@
		// Forked from GOV.UK Frontend, namespace changed to ensure no conflicts.

		$gem-spacing-scale-0: 0;

Move form components into gem #116

Move form components into gem #116

Conversation

NickColley commented Jan 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fofr commented Jan 4, 2018 • edited Loading

NickColley commented Jan 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andysellick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickColley Jan 4, 2018 • edited Loading

Choose a reason for hiding this comment

NickColley commented Jan 3, 2018 •

edited

Loading

fofr commented Jan 4, 2018 •

edited

Loading

NickColley Jan 4, 2018 •

edited

Loading