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

feat: Add gcds-date-input component #607

Merged
merged 37 commits into from
Aug 19, 2024
Merged

feat: Add gcds-date-input component #607

merged 37 commits into from
Aug 19, 2024

Conversation

ethanWallace
Copy link
Collaborator

@ethanWallace ethanWallace commented Aug 8, 2024

Unit tests for new component and functions still need to be added
Requires tokens from cds-snc/gcds-tokens#301

Summary | Résumé

The date input element is a component that comes pre-configured with form elements to gather date information from users.

<gcds-date-input
  name="date" 
  legend="Date input" 
  format="full | compact"
  error-message=""
  required=""
  value=""
  hint=""
  validateOn="blur | submit | other"
  validator=""
  disabled=""
></gcds-date-input>

Required fields

The following are required fields that are needed to render the gcds-date-input.

  • name: The name of the form field
  • legend: The legend text for the fieldset
  • format: The format in which to display. Expects either full or compact

Value format

The gcds-date-input inputs and outputs the dates in the following formats YYYY-MM-DD or YYYY-MM depending on the chosen format. If the value passed to gcds-date-input is not in the given format, the date input will not be pre-filled on load.

Changes

Development of the date input required changes to some other components and the utility functions already used within the GC Design System.

gcds-fieldset

  • Updated CSS to fix padding issue
  • Added check for data-date attribute to handle validation differently.
  • Fixed rendering of (required) text to match Figma design.

gcds-input

  • Added part="input" to internal HTML input to allow styling by parent components.
  • Updated CSS to allow better formatting
  • Updated max-width calculation to display properly
  • Added Watch functions to track aria-invalid and aria-description attributes to allow inheriting of information around validation from parent component.

gcds-select

  • Added part="select" to internal HTML select to allow styling by parent components.
  • Updated CSS to allow better formatting
  • Added Watch functions to track aria-invalid and aria-description attributes to allow inheriting of information around validation from parent component.

inheritAttributes function

  • Removed section of function that removed attributes from host element to allow information around validation to pass information to the inner shadow elements (wasn't working properly anyway)

<Host name={name} onBlur={() => this.onBlur()}>
{this.validateRequiredProps() && (
<gcds-fieldset
legend={legend}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know we had previously agreed to call this prop legend because it is a legend, but after all the discussions we had about fieldsets in the last week, I'm wondering if it would make more sense to name it label to keep it consistent (less confusing?) with other form fields for our users. What do you think @daine @ethanWallace ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm now wondering if it's best to wait on the final decision for the change to the fieldset component for this. But yes I'm leaning towards the fact that it's probably appropriate to call it a label because that's what it is in this case. From what the designers are also proposing, this label also looks different and will have the medium style as opposed to bold. I will tag you in the slack thread where they mentioned it

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just flagging (I don't disagree): If we wait for the final decision on the fieldset component, we will need to hold releasing this component until then. Since the guidance still seems to be requiring some time, it wouldn't be the worst but Abdul did say that one team was waiting on the release of this component.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I could see using label since that is what it appears as and that's what we use on other form components but I still think legend is more appropriate since it is just a legend element styled like a label and doesn't have any extra functionality to mimic label functionality.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good, it is a bit of a weird one so I think either decision is ok!

Copy link
Collaborator

@melaniebmn melaniebmn left a comment

Choose a reason for hiding this comment

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

Left some comments with suggestions. Thank you for putting so much work into this component!

@ethanWallace
Copy link
Collaborator Author

ethanWallace commented Aug 14, 2024

Just realized I might have to rework the isValidDate function (again) since the date input will accept values of either YYYY-MM-DD and YYYY-MM in both formats which it should not do.

FIXED

Copy link
Collaborator

@melaniebmn melaniebmn left a comment

Choose a reason for hiding this comment

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

Ship it!

<Host name={name} onBlur={() => this.onBlur()}>
{this.validateRequiredProps() && (
<gcds-fieldset
legend={legend}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good, it is a bit of a weird one so I think either decision is ok!

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