-
Notifications
You must be signed in to change notification settings - Fork 4
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
Convert the scheme browse and area choose page to govuk style #270
Conversation
@@ -15,12 +16,6 @@ | |||
} | |||
</script> | |||
|
|||
<button type="button" title="Zoom to show entire boundary" on:click={recenter}> | |||
<SecondaryButton title="Zoom to show entire boundary" on:click={recenter}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've checked the main scheme edit page to make sure nothing breaks badly with these changes to shared components. Some styling changes slightly, but not in a bad way. And the next PR will fix everything up there too.
@@ -1,53 +1,13 @@ | |||
<script lang="ts"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm very happy with this one; using the govuk component saves us tons of code
src/lib/common/FileInput.svelte
Outdated
@@ -19,30 +17,15 @@ | |||
} | |||
</script> | |||
|
|||
<!-- TODO Interactive elements inside a label are apparently invalid, but this works --> | |||
<label> | |||
<div class="govuk-form-group"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a design change here. Previously we hid the filename of whatever the user picks. In the 3 cases where it's used currently, I don't think this is so important, and the govuk style very much does not hide the filename
<script lang="ts"> | ||
// The label to show | ||
export let label: string; | ||
// A unique (per page) ID of the form element being labelled. Something in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish we didn't have to make this up at all. Previously we've been doing <label>xyz <input ... /></label>
, but this breaks the govuk styling by eliminating a line-break. So, this Svelte component slightly helps.
@@ -0,0 +1,34 @@ | |||
<script lang="ts"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This component is very necessary. If you check the HTML sample at https://design-system.service.gov.uk/components/radios/, it's so much code to copy. Wrapping in a Svelte component is quite similar to their Nunchucks system
>open source project</ExternalLink | ||
> supported by Active Travel England and developed by: | ||
</p> | ||
<div class="govuk-prose"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's an example of auto-applying CSS classes to everything below. All the diff below is just introducing one level of nesting in the DOM
<p>Authority or region: {scheme.authority_or_region}</p> | ||
<p>Capital scheme ID: {scheme.capital_scheme_id}</p> | ||
<p>Funding programme: {scheme.funding_programme}</p> | ||
<div class="govuk-button-group"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made two of the buttons side-by-side. I'm sure the contents of these expandable cards will keep evolving, so not changing much else yet
data-testid="transport-authority" | ||
list="authorities-list" | ||
bind:value={inputValue} | ||
<div class="govuk-grid-row"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using https://design-system.service.gov.uk/styles/layout/ to split the page into two halves
id="inputValue" | ||
> | ||
<input | ||
class="govuk-input govuk-input--width-20" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's a case where it's simplest to just directly use govuk classes, rather than refactor a component yet. I couldn't find any guidance on text input fields with custom autocomplete like this.
<Radio | ||
legend="Or pick from the map" | ||
id="showBoundaries" | ||
choices={[ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This used to be a select/dropdown, but https://design-system.service.gov.uk/components/select/ recommends against, and I agree here
Based on the video bit 👍 to this, looks really good. Great work Dustin. |
Interested to hear what @Pete-Y-CS, @Sparrow0hawk or others think also.. |
@@ -0,0 +1,9 @@ | |||
<button | |||
type="button" | |||
class="govuk-button" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we want to share this with SecondaryButton
and just pass isSecondary
as an optional param?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think this needs to hold up this merge but could be nice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about that, but not sure yet. https://design-system.service.gov.uk/components/button/ recommends avoiding multiple "default buttons" per page, so making it extremely clear what type of button is used seems nice. We could definitely do <Button kind="default|secondary">
too to share a bit of code, but maybe it makes the places used a little harder to read.
src/lib/common/FileInput.svelte
Outdated
<!-- TODO Interactive elements inside a label are apparently invalid, but this works --> | ||
<label> | ||
<div class="govuk-form-group"> | ||
<label class="govuk-label" for="file-upload">{label}</label> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we be using the form element defined below here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah good point, thanks! Fixed. I also brought back the unique ID, because <label>
will break otherwise. We don't have any page with multiple file inputs, but we'll need to when we do
…two file inputs on one page
This is the next step to use the govuk design system, on our two simpler pages. I've tried my best to interpret https://design-system.service.gov.uk/styles/ and https://design-system.service.gov.uk/components/, but I (really hope!) we go through some kind of design review later, because there are plenty of unclear decisions made.
Compare: old homepage with new homepage, and old browse with new browse
screencast.mp4
In the step after this one, I'll convert our main edit page... or at least the sidebar. I haven't found much guidance yet for design patterns on maps, such as tooltips, changing cursors, hover effects, etc -- lots of the questions we've been asking anyway! I'll search/ask around to see if there's any guidance here yet.
Code style choices
Copying the sample HTML directly from govuk docs would severely pollute our codebase, and make it tough to check that we've converted all elements on every page to using the right CSS classes. So there are 3 ways we use govuk components:
<div class="govuk-prose">