-
Notifications
You must be signed in to change notification settings - Fork 2
Add a review element #108
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
base: 1.x
Are you sure you want to change the base?
Add a review element #108
Conversation
|
Got to say while it is called 'summary list', I prefer 'review element' for the webform component, because that's more clear as it stands? I'm going to take this out of draft, as that last push cleared the only bug I could find. |
It wasn't until I clicked the gov.uk example that I understood what this was... Review element feels much nicer. Summary list was conjuring up sidebar menus for some reason, but then my head is a funny place. |
|
@ekes is it ONLY for a webform review? |
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.
Let's change the file name for the CSS to use hyphens instead of underscores.
| @@ -0,0 +1,41 @@ | |||
| .webform-localgov-review-element { | |||
| display: flex; | |||
| margin-bottom: var(--spacing); | |||
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.
These variables are only available when localgov_base is used as the base theme. If this component is used anywhere independently, then these variables will all return empty.
Let's update this to use hardcoded values of 1rem instead of var(--spacing) and 1px solid #cecfd0 for border.
We can create a follow-up issue in localgov_base to rewrite this CSS using variables that subthemes can override.
| - webform/webform.composite | ||
|
|
||
| localgov_forms_review_element: | ||
| version: VERSION |
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.
Remove version, or else it will be cached until the version of Drupal core that is being used is updated.
|
Might be misinterpreting this, as I can set up separate review elements. Although I still have the bug of it only going back to the last page. Given how big some of our forms can get, I'd consider it quite helpful to have review elements auto generated for all. |
|
Bug: it does seem to go back the incorrect (or always previous) page. Automatically generating the review elements. I'm not sure how that'd be possible. Well, I guess, for a simple form with nothing hidden, no conditionals, and only straight fields (no hidden, calculated etc.) But as soon as something is computed, which do you show - what the user entered or what you calculated - which do you link back to; or if something is conditional you need to reimplement the conditional automatically etc. |
|
We had an issue at Greenwich with not linking to the correct item, which we fixed by turning off ajax on the form (iirc). |
Think switching ajax off on the form I'm working with isn't going to be one people want (lots of dynamically calculated stuff). I assume that means we need in addition to the link, some equivalent js to do the ajax link back :-/ |
| </div> | ||
| <div class="webform-localgov-review-element__value">{{ element['#value'] }}</div> | ||
| {% if element.wizard_prev %} | ||
| <div class="webform-localgov-review-element__change">{{ element.wizard_prev }}</div> |
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.
It looks like wizard_prev is the reason we get taken back to the previous page as opposed to the source_page


Following https://design-system.service.gov.uk/components/summary-list/ pattern to make a summary of the form entries and link back to change them.
Inital port of the Review Element from @tanc at OpenCode for Greenwich.
Couple of questions about it, and maybe some example, and then it's ready to go. Maybe should rename to Summary Element (?)