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(Forms): expiry field #2660

Merged
merged 94 commits into from
Nov 15, 2023
Merged

feat(Forms): expiry field #2660

merged 94 commits into from
Nov 15, 2023

Conversation

joakbjerk
Copy link
Contributor

@joakbjerk joakbjerk commented Sep 15, 2023

  • Add tests
  • Add type documentation
  • Add portal documentation
  • Fine tune toggle functionality
  • Feedback to user / changes to input behavior
  • implement useDataValue hook and SharedContext

@vercel
Copy link

vercel bot commented Sep 15, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
eufemia ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 14, 2023 2:59pm

@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 15, 2023

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 7e45d6a:

Sandbox Source
eufemia-starter Configuration

@joakbjerk joakbjerk force-pushed the feat/expiry-date-field branch from f740cf0 to c3235b6 Compare September 26, 2023 12:22
@joakbjerk joakbjerk marked this pull request as ready for review September 27, 2023 12:59
@joakbjerk joakbjerk force-pushed the feat/expiry-date-field branch from edd3c18 to c116ce0 Compare September 27, 2023 13:02
}
input_element={
<span className="dnb-date-picker__input__wrapper">
<ExpiryDateField
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be possible to achieve the UI in a simpler way using InputMasked?

Copy link
Contributor

Choose a reason for hiding this comment

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

The Field.String component support mask-prop by the way, leading to the inner implementation using InputMasked instead of Input. Maybe that can be used, and then just wrapped with a parser that "translates" from the "UI value" (like "01/01") to the external value of the Expiry component (the object structure with the two values separate)? That can probably be done in the fromInput / toInput derivates, getting all the other features automatically (like the distribution of props to FieldBlock etc inside Field.String

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did start out with the Field.String and a custom mask, but this ended up complicating the process of making the tab-behaviour function like it does in the DatePicker-component, and keeping the placeholder text while the user is typing. And as far is I could tell, it did not support labels for the different values either, like the DatePicker component does for day month year etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

But if Field.String lacks some features you need, could this possibly be simplified at least a bit using InputMasked instead of creating a "custom" mask feature?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm not sure if I quite understand? I tried using a InputMasked too, but it would no let me keep the placeholder while typing, and then there is the issue with the tab-functionality and missing labels for month and year. I found the simplified version I tried out, a bit lacking in the accessibility department.

That is why I went with the same approach as done in the DatePickerInput component, and the TextMask component is also imported from there

Copy link
Contributor

Choose a reason for hiding this comment

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

There might be specific needs here that makes it hard to use those components, but it just seems to me that this is something the InputMasked component should have features for, so we don't need to write and maintain custom code for this kind of UI details in a component like this one. It should be standardized, so maybe we need to change/extend the features of InputMasked?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah totally agree! I don't see why the functionality that I needed from TextMask, couldn't be integrated into InputMasked. As of now, it looks like that TextMask is only used internally for the DatePickerInput. @tujoworker any thoughts?

Choose a reason for hiding this comment

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

Sorry to un-resolve this: we were the ones who suggested adding the Expiry Date component. https://jira.tech.dnb.no/browse/EDS-571

With regards to @henit's arguments: ironically, the closest we (consumers of eufemia) could "easily" come to the desired behaviour was with an <InputMasked />..

      <InputMasked
        className={style.inputMasked}
        mask={[/\d/, /\d/, '/', /\d/, /\d/]}
        mask_options={{
          allowNegative: false,
        }}
        on_change={({ cleanedValue }) => setInput(cleanedValue)}
        show_mask
        /**
         * The character U+2013 "–" could be confused with the ASCII character
         * U+002d "-", which is more common in source code.
         * This is here for screen readers and to allow usage of input.length from
         * the cleanedValue of the on_change.
         */
        placeholder_char='–'
        size='large'
        align='center'
        top='medium'
      />

@joakbjerk
Copy link
Contributor Author

Here's a few observations when testing the component https://eufemia-git-feat-expiry-date-field-eufemia.vercel.app/uilib/extensions/forms/feature-fields/

In Google Chrome, I get the following error/warning in the docs(not sure if this is critical or not):

Duplicate form field id in the same form
Multiple form field elements in the same form have the same id attribute value. This might prevent the browser from correctly autofilling the form.
To fix this issue, use unique id attribute values for each form field.
12 resources

This issue should be fixed now 🙏

@tujoworker tujoworker self-requested a review November 14, 2023 09:45
Copy link
Member

@tujoworker tujoworker left a comment

Choose a reason for hiding this comment

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

Nice work! Just some request we need to look at. And some we need to fix before merge.

  • Also, the width of the field should be width: var(--forms-field-width--small); while the.
  • And justify-content: center; in the .dnb-input__shell I think will be correct.
  • One visual test would be good as well 🙏


## Properties

<DataValueReadwriteProperties type="string" />
Copy link
Member

Choose a reason for hiding this comment

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

Are there any properties we should omit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, will double check!

@joakbjerk joakbjerk merged commit af5aa61 into main Nov 15, 2023
7 checks passed
@joakbjerk joakbjerk deleted the feat/expiry-date-field branch November 15, 2023 07:54
tujoworker pushed a commit that referenced this pull request Nov 16, 2023
## [10.14.0](v10.13.0...v10.14.0) (2023-11-16)

### 📝 Documentation

* **Forms:** lists component specific props 1st ([#2892](#2892)) ([c05740c](c05740c))
* **PhoneNumber:** remove unsupported props ([#2894](#2894)) ([4800a8e](4800a8e))
* **PostalCodeAndCity:** remove unsupported props ([#2890](#2890)) ([fe1ee9e](fe1ee9e))

### ✨ Features

* **Forms:** expiry field ([#2660](#2660)) ([af5aa61](af5aa61))
* **forms:** improved state management and reacting to more changed props ([#2882](#2882)) ([0ca9533](0ca9533))
* **Input:** add clear button event "on_clear" ([#2898](#2898)) ([eb6b722](eb6b722))
* **Radio:** Sbanken styling ([#2888](#2888)) ([d7ffcf8](d7ffcf8))

### 🐛 Bug Fixes

* add "use client" to non hook components like the Button ([#2895](#2895)) ([2d54a13](2d54a13))
* **Autocomplete:** enhance logic for when to blur ([#2886](#2886)) ([ce5c3fa](ce5c3fa))
* **Autocomplete:** make clear button work with enter key ([#2901](#2901)) ([30007c4](30007c4)), closes [#2185](#2185)
* **FieldBlock:** enhance fieldset/legend detection ([#2902](#2902)) ([4c62052](4c62052)), closes [#2893](#2893)
* fix Flex and Grid export to work with Vite.js ([#2905](#2905)) ([ef83713](ef83713))
* fix vertical label_direction support for Radio group and ToggleButton group ([#2899](#2899)) ([d650c66](d650c66))
* **forms:** Field block error handling ([#2900](#2900)) ([9582c64](9582c64))
* **forms:** Improved message value replacements ([#2903](#2903)) ([a61140b](a61140b))
* **PhoneNumber:** keep selected countryCode value on blur ([#2869](#2869)) ([7e0f9c5](7e0f9c5))
@tujoworker
Copy link
Member

🎉 This PR is included in version 10.14.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

5 participants