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

Update and add form components #75

Merged
merged 4 commits into from
Nov 23, 2022
Merged

Conversation

VirginiaDooley
Copy link
Contributor

@VirginiaDooley VirginiaDooley commented Nov 7, 2022

Ref https://trello.com/c/lhcjgP0s/3121-review-chriss-ee-design-feedback and #74

This work:

  • removes the word wrap when the text in a table is code

Screen Shot 2022-11-10 at 5 06 53 PM

  • makes radio button selections clear and WCAG compliant
    Screen Shot 2022-11-07 at 6 52 11 PM
  • adds a date form component
    image

These changes have been tested in EE per screenshots above.

@VirginiaDooley
Copy link
Contributor Author

@VirginiaDooley VirginiaDooley force-pushed the hotfix/extend-components branch from e60ecbb to 7891d6c Compare November 7, 2022 14:38
@VirginiaDooley VirginiaDooley marked this pull request as ready for review November 7, 2022 15:21
@VirginiaDooley VirginiaDooley force-pushed the hotfix/extend-components branch from 947db39 to 020efb4 Compare November 7, 2022 18:51
@VirginiaDooley VirginiaDooley changed the title Add more flexibility in existing components Update and add components Nov 8, 2022
@VirginiaDooley VirginiaDooley changed the title Update and add components Update and add form components Nov 8, 2022
@Bekabyx
Copy link
Contributor

Bekabyx commented Nov 11, 2022

Any chance we could include this component on either layout-demo or /usage/composition/? Those are the two pages lighthouse is going to be run against so it would be nice to have :)

@VirginiaDooley VirginiaDooley force-pushed the hotfix/extend-components branch from bc4b2ca to e72f317 Compare November 14, 2022 10:03
@VirginiaDooley
Copy link
Contributor Author

VirginiaDooley commented Nov 14, 2022

Any chance we could include this component on either layout-demo or /usage/composition/? Those are the two pages lighthouse is going to be run against so it would be nice to have :)

Added here e72f317

@symroe
Copy link
Member

symroe commented Nov 21, 2022

Date picker

Some smaller points first:

  • Can you call this a date picker, rather than date form? This is because the picker might be on a larger form, so the wording might get confusing.
  • The frontmatter in the date-form.md file is missing the initial ---. This means the menu item isn't showing up on the components sidebar on the site.
  • The site is also not building the CSS for the date picker. This is because you need to include it in system/docs.scss (twice, import the mixin at the top, then add it to optional-styles. I actually forgot to do this on my last PRs for switcher and showcase, so do you mind adding these too?
  • The formatting is off on the docs page for the date picker. I think this is due to blank lines between the HTML sections. Remove these lines and it'll work ok

Now, zooming out to the content. There is a load of CSS in the mixin at the moment, including the form styling, legends, etc. Can you remove all of this as it's just duplicated?

The HTML in the markdown file isn't really in line with the other form HTML we have in the design system (it looks more like the old base theme HTML to me). I think this can be simplified to:

<div class="ds-scope site-resizer">
  <div class="ds-date">
    <div class="ds-field">
      <label for="day_0">Day</label>
      <input type="number" name="day_0" value="12" required="" id="id_date-date_0">
    </div>
    <div class="ds-field">
      <label for="month_1">Month</label>
      <input type="number" name="month_1" value="12" required="" id="id_date-date_1">
    </div>
    <div class="ds-field">
      <label for="year_2">Year</label>
      <input type="number" name="year_2" value="2025" required="" id="id_date-date_2">
    </div>
  </div>
</div>

That is, wrapping a load of ds-fields in a ds-date class.

In turn, the ds-date can be reduced to:

@mixin date-form {
  @extend %ds-cluster;
  .ds-date {
    .ds-field {
      input {
        margin: 10px 15px 5px 0;
        border: 2px solid $black;
        padding: $s1;
        max-width: 5em;
        color:$black;
      }
      &:last-child input {
        max-width: 6em;
      }
    }
  }
}

Not that I'm extending from ds-cluster here, so we can group the fields using the existing cluster code.

I'm also targeting the last-child and assuming this is a year field. I think this is better than relying on a hard coded ID as you had (what if we needed more than one date-picker on a page?), but it's not the best. We might want to use a CSS class in the HTML, but I thought that might end up being tricky in Django's forms. For now I think last-child is going to be ok.

@VirginiaDooley VirginiaDooley force-pushed the hotfix/extend-components branch 3 times, most recently from 6376d06 to 122b223 Compare November 21, 2022 15:23
@VirginiaDooley
Copy link
Contributor Author

VirginiaDooley commented Nov 21, 2022

Date picker

Some smaller points first:

  • Can you call this a date picker, rather than date form? This is because the picker might be on a larger form, so the wording might get confusing.
  • The frontmatter in the date-form.md file is missing the initial ---. This means the menu item isn't showing up on the components sidebar on the site.
  • The site is also not building the CSS for the date picker. This is because you need to include it in system/docs.scss (twice, import the mixin at the top, then add it to optional-styles. I actually forgot to do this on my last PRs for switcher and showcase, so do you mind adding these too?
  • The formatting is off on the docs page for the date picker. I think this is due to blank lines between the HTML sections. Remove these lines and it'll work ok

Now, zooming out to the content. There is a load of CSS in the mixin at the moment, including the form styling, legends, etc. Can you remove all of this as it's just duplicated?

The HTML in the markdown file isn't really in line with the other form HTML we have in the design system (it looks more like the old base theme HTML to me). I think this can be simplified to:

<div class="ds-scope site-resizer">
  <div class="ds-date">
    <div class="ds-field">
      <label for="day_0">Day</label>
      <input type="number" name="day_0" value="12" required="" id="id_date-date_0">
    </div>
    <div class="ds-field">
      <label for="month_1">Month</label>
      <input type="number" name="month_1" value="12" required="" id="id_date-date_1">
    </div>
    <div class="ds-field">
      <label for="year_2">Year</label>
      <input type="number" name="year_2" value="2025" required="" id="id_date-date_2">
    </div>
  </div>
</div>

That is, wrapping a load of ds-fields in a ds-date class.

In turn, the ds-date can be reduced to:

@mixin date-form {
  @extend %ds-cluster;
  .ds-date {
    .ds-field {
      input {
        margin: 10px 15px 5px 0;
        border: 2px solid $black;
        padding: $s1;
        max-width: 5em;
        color:$black;
      }
      &:last-child input {
        max-width: 6em;
      }
    }
  }
}

Not that I'm extending from ds-cluster here, so we can group the fields using the existing cluster code.

I'm also targeting the last-child and assuming this is a year field. I think this is better than relying on a hard coded ID as you had (what if we needed more than one date-picker on a page?), but it's not the best. We might want to use a CSS class in the HTML, but I thought that might end up being tricky in Django's forms. For now I think last-child is going to be ok.

Thanks for taking the time to break this down. I had overlooked the possibility if extending an existing component so will bear that in mind next time we have a need for a new component. All your edits should now be addressed in 7297850.

@VirginiaDooley VirginiaDooley force-pushed the hotfix/extend-components branch 4 times, most recently from 7ad8b87 to 8a20b15 Compare November 22, 2022 13:51
@VirginiaDooley VirginiaDooley force-pushed the hotfix/extend-components branch 2 times, most recently from c980896 to ade602a Compare November 22, 2022 16:40
@VirginiaDooley VirginiaDooley force-pushed the hotfix/extend-components branch 2 times, most recently from 703838a to 49a5d19 Compare November 23, 2022 13:49
@VirginiaDooley VirginiaDooley force-pushed the hotfix/extend-components branch from 49a5d19 to 656a94e Compare November 23, 2022 13:49
Copy link
Member

@symroe symroe left a comment

Choose a reason for hiding this comment

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

One minor comment, but this looks good to me!

Thanks for adding the docs about making a new component too 👍

@@ -0,0 +1,17 @@
@mixin date-picker {
Copy link
Member

Choose a reason for hiding this comment

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

Very minor, but can you fix the indentation on this file?

@VirginiaDooley VirginiaDooley force-pushed the hotfix/extend-components branch from 7ae7412 to afc09d9 Compare November 23, 2022 15:08
@VirginiaDooley VirginiaDooley merged commit 83bb514 into master Nov 23, 2022
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.

3 participants