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 sd-input #255

Closed
26 of 43 tasks
Tracked by #242
mariohamann opened this issue Jul 26, 2023 · 46 comments · Fixed by #515
Closed
26 of 43 tasks
Tracked by #242

feat: ✨ add sd-input #255

mariohamann opened this issue Jul 26, 2023 · 46 comments · Fixed by #515
Assignees
Labels
CL-migration All components which need to be migrated from Component Library 🎨 figma needs changes in Figma
Milestone

Comments

@mariohamann
Copy link
Contributor

mariohamann commented Jul 26, 2023

Description

As a user of the Solid Design System, I would like to use an Input component that allows me to enter text. This Input component should have the flexibility to be displayed in various ways, depending on the specified type attribute. Additionally, I should have the option to hide the default label, display a hint if needed, and have mandatory fields clearly marked with an * added to their label.

API

https://www.figma.com/file/xSIeTnyfW2T21Uw5JgdZOg/Input?node-id=0%3A1&mode=dev

Handoff

https://www.figma.com/file/xSIeTnyfW2T21Uw5JgdZOg/Input?node-id=6%3A4&mode=dev

Dev notes

Components detailed requirements

Here is a WIP branch from Design with all adoptions resulting from the implementation.

Props

the following props only relate to design relevant props. All other props that are already provided (e.g. type, pattern, min max etc.) should not be removed.

Status Name Type Default Description
clearable boolean false shows the clear icon on the right end which clears the input field when clicked
disabled boolean false disables the input field
helper-text string `` shows the hint message underneath the input field
placeholder string `` adds plain text to the placeholder inside the input field
error-text string `` adds plain text to the errormessage underneath the helper text
label string `` adds plain text to the label above the input field
size lg / md / sm lg defines the height of the input field

Parts

  • all existing one's from Shoelace
  • error-text

Slots

  • label - slot for the input field label
  • helper-text - slot for the helper text
  • error-text - slot for the error message
  • left - slot to add prefix content to the start of the input field
  • right - slot to add suffix content to the end of the input field
  • placeholder - slot for the placeholder of the input field

Stories (besides the default story)

  • Sizes & Icons - sizes, icons, clearable
  • Descriptions - helper text, error text, placeholder & label

Samples

  • tooltip - An input field with a sd-interactive (using info-circle) added to the label
  • stepper

Open Questions towards design

  • @mariohamann / @coraliefeil if we activate the tooltip, how does the content for the tooltip will be added? dont we need a slot for this?
  • @mariohamann / @coraliefeil is the icon-right property really true by default? => no/maybe. @van-nguyen-ht clarifies that and adds a comment here if needed: the field input with the icon-right true by default is the auto-suggest with the search icon.
  • @coraliefeil the message property is not available in the playground. Do you know why? => @van-nguyen-ht verifies it. ??
  • @solid-design-system/design what is the cursor property about? This seems like a pure design property (might just need an asterisk). => yes. @van-nguyen-ht is changing it.
  • @coraliefeil / @van-nguyen-ht pls make sure the figma file is updated or we have a link to the respective branch in this ticket. There are several mismatching information in this ticket and the current figma file regarding the API.
  • @mariohamann pls go through my changes and check if I missed something.
  • @mariohamann pls define the samples we need for this component.
  • @mariohamann pls define which parts from shoelace we need and change the list above accordingly.

DoR (Definition of Ready)

  • This user story provides value to the product.
  • The development team has estimated this user story.
  • The user story is clear and well-defined.
  • Any dependencies related to this user story have been identified.

DoD (Definition of Done)

  • Documentation for this Input component has been created or updated if applicable.
  • A migration guide is available or updated if applicable.
  • Relevant end-to-end tests for features, accessibility, and bug fixes are created or updated.
  • Relevant stories for features and accessibility are created or updated.
  • Implementation successfully works on the feature branch.
@mariohamann mariohamann added this to the Form elements milestone Jul 26, 2023
@karlbaumhauer karlbaumhauer removed this from the Forms milestone Jul 27, 2023
@yoezlem yoezlem moved this from 📋 Backlog to To be refined in Solid Design System Project Board Sep 28, 2023
@coraliefeil
Copy link
Contributor

coraliefeil commented Sep 29, 2023

I would like to adjust the figma comp according to code comp once it is ready.

We might see things that need to be adjusted while coding.

E.g. the default slot content for the field slot could be "placeholder" but turned off - > field es empty in figma per default
...
We need to clarify once again the icon-left ... do we offer it or not. -> Edit: We see teams needing icon-left and we synced it with brand so keeping icon left in comp as we have it is fine.

@yoezlem yoezlem moved this from To be refined to 🔖 Ready in Solid Design System Project Board Sep 29, 2023
@abudd1094 abudd1094 self-assigned this Oct 9, 2023
@abudd1094 abudd1094 moved this from 🔖 Ready to 🏗 In progress in Solid Design System Project Board Oct 9, 2023
@coraliefeil
Copy link
Contributor

For the tooltip-icon -> sd-interactive is being used, right?

@abudd1094
Copy link
Contributor

abudd1094 commented Oct 17, 2023

@mariohamann @karlbaumhauer @yoezlem
This ticket as it is written is difficult to interpret in detail.

So I understand:

  • Dev Note 1: "we start with https://github.com/solid-design-system/solid/tree/feat/add-form-elements" --- What exactly are the "needed form files" ? These files from the link that are included in Özlem's sd-radio branch?
    image
  • Dev Note 2: _components folder is what exactly? A private folder of starting points based on Shoelace? These have already been tailored or are copied and pasted directly from Shoelace?
    I moved the _components/input into components and it is quite large and complicated. Is all this coming directly from Shoelace or have you previously worked on it? Am I expected to keep all the stories and tests as is?
    image

I will follow the Dev Notes to the best of my ability. Seems like there are many points left undiscussed and a lot will come up in review. Is this really ready to implement?

@coraliefeil good question, I would also like to know.

@mariohamann I'm seeing TS errors out of the box that are also included in other existing components like:
image
which is also highlighted as an error in alert, dialog, and others... Is there something I need to configure to make sure my linting is working the same as yours?

@karlbaumhauer
Copy link
Contributor

@abudd1094 I will try to answer them as good as I am able to... ;-)

@mariohamann @karlbaumhauer @yoezlem This ticket as it is written is difficult to interpret in detail.

So I understand:

"the needed form files" are basically everything you need to get the input component running. With more complex components, there are usually multiple "extracted" JS, which needs to be imported. So if you copy the from /_components/... to /components/..., you need to make sure you also copy all necessary dependencies as well. I think @Vahid1919 did this already and might be able to help here.

  • Dev Note 2: _components folder is what exactly? A private folder of starting points based on Shoelace? These have already been tailored or are copied and pasted directly from Shoelace?
    I moved the _components/input into components and it is quite large and complicated. Is all this coming directly from Shoelace or have you previously worked on it? Am I expected to keep all the stories and tests as is?

So the _components folder is basically the base coming from shoelace. Every time we start a new component, we copy the component folder and all the dependencies and try to make it run. After that, we need to adopt union investment styling and maybe even stuff to the component API (according to the info in Figma / the ticket).

The tests and stories need then to be adopted according to our need from the ticket. Unfortunately I just realized that you (again) got a ticket which is quite old and we haven't been very precise and structured in the beginning with the tickets. I will try to reformulate this asap.

I will follow the Dev Notes to the best of my ability. Seems like there are many points left undiscussed and a lot will come up in review. Is this really ready to implement?

As this is most probably one of the most complex components in terms of a11y, testing and dependencies, I am pretty sure that the review will be pretty big anyways. If you don't feel blocked by any information, I suggest to start and see how far you will come (after I refactored the description) and then just face the bigger review later on.

@coraliefeil good question, I would also like to know.

pls yes. Do so if possible.

@mariohamann I'm seeing TS errors out of the box that are also included in other existing components like: image which is also highlighted as an error in alert, dialog, and others... Is there something I need to configure to make sure my linting is working the same as yours?

@solid-design-system/development can somebody else help out with the linting here?

@karlbaumhauer karlbaumhauer added the 🎨 figma needs changes in Figma label Oct 18, 2023
@karlbaumhauer
Copy link
Contributor

@abudd1094 I updated the description but found some (at least for me) unclear points. Tagged Coralie and Mario to help answering these questions.
Lets see if we can clarify this tomorrow in the daily.

@mariohamann
Copy link
Contributor Author

Regarding tooltip: I don't see any value in providing a tooltip here. They can slot it without any additional effort.

If we implement it, we instead have to create a tooltip slot, integrate tooltip-related stuff like positioning etc. Let's please remove this from the component and let it just be a sample.

@coraliefeil

@mariohamann
Copy link
Contributor Author

I updated the props table to my understanding: Removed icon-left and icon-right (is just handled via their slot), tooltip (creates unneeded overhead for an already complex component), changed label, help-text and placeholder tonstring instead of boolean.

@karlbaumhauer

@mariohamann
Copy link
Contributor Author

Updated parts. Regarding samples: What do we have from design? I would match those from design, at least one with a tooltip.

@mariohamann
Copy link
Contributor Author

In general: I hope that the message-thing will work out. 🤞

@mariohamann mariohamann removed their assignment Oct 18, 2023
@coraliefeil
Copy link
Contributor

Only one sample so far ... stepper-values. Can add label-tooltip sample.

I’ve opened up a branch to refactor sd-input design comp while coding - listing down all new decisions

@coraliefeil
Copy link
Contributor

We did not define anything specific for A11y aspects for form elements yet ... especially when sth is invalid.

This article has excellent suggestions smashingmagazine.com/2023/02/guide-accessible-form-validation/#invalid-fields

@solid-design-system/development FYI

@abudd1094
Copy link
Contributor

@karlbaumhauer @mariohamann
There are additional props like 'pill' that seem to have no relevance in our design and have no visual effect.
We noted above that existing props should not be removed.

Probably this was not yet considered. Should I systematically document what I think is relevant as I go (to discuss) or simply remove it on my own? In the case of pill, there is nothing in Figma, it has no visual effect, I would delete it and its corresponding style if left to my own devices...

@abudd1094
Copy link
Contributor

abudd1094 commented Oct 19, 2023

@mariohamann

What exactly is form-control.styles.ts? Is it only present for the migration of Shoelace components or does it serve a purpose? --sd-input-label-font-size-small seems to only exist there but the inclusion of sd-input makes me think it is intended for our use... perhaps just a placeholder...

image

I'll mirror what Özlem did for the radio-group and move those to the css literal in the sd-input component:
image

@karlbaumhauer
Copy link
Contributor

karlbaumhauer commented Oct 19, 2023

Should I systematically document what I think is relevant as I go (to discuss)

@abudd1094 That would be awesome.

@van-nguyen-ht
Copy link
Contributor

@mariohamann I have a question towards development the behavior of the input field: will the icon be the stop for the text (where it breaks) in which the field's width is fix or will it get "pushed" by the text and the whole field grow with?

@van-nguyen-ht
Copy link
Contributor

@coraliefeil @yoezlem might be a redundant question: should the successful validation be named success-message or?

@coraliefeil
Copy link
Contributor

coraliefeil commented Oct 24, 2023

success-text sounds good now that we have an error-text

@coraliefeil
Copy link
Contributor

@mariohamann may I ask why hint needs a separate slot?

... cause it’s all slotted and hint text is just the default slot content

@coraliefeil
Copy link
Contributor

coraliefeil commented Oct 25, 2023

I’ve updated the issue description according to our agreements from today. Also updated most things in figma and left some comments for @van-nguyen-ht to do.

@abudd1094
Copy link
Contributor

abudd1094 commented Oct 26, 2023

@mariohamann

image

storybookjs/storybook#23999

image

Think we can solve this error by upgrading Storybook
image

@abudd1094
Copy link
Contributor

abudd1094 commented Oct 27, 2023

@solid-design-system/development @karlbaumhauer @mariohamann
I am deeply confused about how Shoelace applies its "invalid" styles and how deep I need to go in our sd-input.

@yoezlem uses an "invalid" property in sd-radio, but that does not exist in sl-radio:
/** A Boolean attribute which, if present, marks the radio Button valid or invalid */ @property({ type: Boolean, reflect: true }) invalid = false;

This is then used to apply the conditional "invalid" styles via tailwind:
<span part="checked" class=${cx( 'rounded-full inline-flex text-white border bg-accent h-2.5 w-2.5', (this.disabled && 'bg-neutral-500') || (this.invalid && 'bg-error hover:bg-error-400 group-hover:bg-error-400') || (this.checked && 'bg-accent hover:bg-accent-550 group-hover:bg-accent-550') || 'bg-neutral-800' )} ></span>

Should I include the same additional property for sd-input (sl-input also does not use an "invalid" property) and mirror this approach?

What bothers me at the moment, is I have a demo form in a temporary story (used for development) in which the invalid styles are immediately applied before the user touches anything (using a simple sd-input with the "required" property). I assume this is because behavior around input "touching" (user must first click input before invalid styles are applied) is then done by the developer using our component when implementing their form?

Shoelace has a nice example where they apply the styles conditionally only after submission. But I can't understand how that works from the code there: https://shoelace.style/getting-started/form-controls#styling-invalid-form-controls.

My assumption is I am going too deep testing that behavior. I should simply use an "invalid" property on the sd-input component itself (even though Shoelace does not do that) and then it is the implementers job to make sure the invalid property is properly controlled from the form parent... Does this sound right? Can anyone please advise?

@Vahid1919
Copy link
Contributor

Vahid1919 commented Oct 27, 2023

Should I include the same additional property for sd-input (sl-input also does not use an "invalid" property) and mirror this approach?

What bothers me at the moment, is I have a demo form in a temporary story (used for development) in which the invalid styles are immediately applied before the user touches anything (using a simple sd-input with the "required" property). I assume this is because behavior around input "touching" (user must first click input before invalid styles are applied) is then done by the developer using our component when implementing their form?

Shoelace has a nice example where they apply the styles conditionally only after submission. But I can't understand how that works from the code there: https://shoelace.style/getting-started/form-controls#styling-invalid-form-controls.

My assumption is I am going too deep testing that behavior. I should simply use an "invalid" property on the sd-input component itself (even though Shoelace does not do that) and then it is the implementers job to make sure the invalid property is properly controlled from the form parent... Does this sound right? Can anyone please advise?

@abudd1094 I think I would go with the "invalid" property simply because that's the approach that was already implemented in another form. We've also added properties that don't exist in shoelace(eg: sd-carousel, if I remember correctly) if it better suits our use case, so adding a brand new attribute is normal I would say.

@karlbaumhauer
Copy link
Contributor

My assumption is I am going too deep testing that behavior. I should simply use an "invalid" property on the sd-input component itself (even though Shoelace does not do that) and then it is the implementers job to make sure the invalid property is properly controlled from the form parent... Does this sound right? Can anyone please advise?

I am not 100% sure but it sounds a bit like it. I would say yes. Just build in the invalid prop and leave it to the user to control it. As we dont offer a form with this component but only the input field, I would only implement the logic of the input field => showing that the input is invalid.

right @mariohamann ?

@mariohamann
Copy link
Contributor Author

@solid-design-system/development @karlbaumhauer @mariohamann I am deeply confused about how Shoelace applies its "invalid" styles and how deep I need to go in our sd-input.

@yoezlem uses an "invalid" property in sd-radio, but that does not exist in sl-radio: /** A Boolean attribute which, if present, marks the radio Button valid or invalid */ @property({ type: Boolean, reflect: true }) invalid = false;

This is then used to apply the conditional "invalid" styles via tailwind: <span part="checked" class=${cx( 'rounded-full inline-flex text-white border bg-accent h-2.5 w-2.5', (this.disabled && 'bg-neutral-500') || (this.invalid && 'bg-error hover:bg-error-400 group-hover:bg-error-400') || (this.checked && 'bg-accent hover:bg-accent-550 group-hover:bg-accent-550') || 'bg-neutral-800' )} ></span>

Should I include the same additional property for sd-input (sl-input also does not use an "invalid" property) and mirror this approach?

What bothers me at the moment, is I have a demo form in a temporary story (used for development) in which the invalid styles are immediately applied before the user touches anything (using a simple sd-input with the "required" property). I assume this is because behavior around input "touching" (user must first click input before invalid styles are applied) is then done by the developer using our component when implementing their form?

Shoelace has a nice example where they apply the styles conditionally only after submission. But I can't understand how that works from the code there: https://shoelace.style/getting-started/form-controls#styling-invalid-form-controls.

My assumption is I am going too deep testing that behavior. I should simply use an "invalid" property on the sd-input component itself (even though Shoelace does not do that) and then it is the implementers job to make sure the invalid property is properly controlled from the form parent... Does this sound right? Can anyone please advise?

@abudd1094 No, validation has to work out of the box. Like it already does. Radio behaves a bit different, as it needs custom validation because of ShadowDOM and it depends on other elements which are in ShadowDOM either.

Text input has to work on its own.

@DanielHargesheimer
Copy link
Contributor

@mariohamann, the validation will work out of the box, but not the error styles. The error styles depend on the invalid prop.

@mariohamann
Copy link
Contributor Author

https://tailwindcss.com/docs/hover-focus-and-other-states#form-states

Did someone try this in the components? (Not radio, as it's special.)

@abudd1094
Copy link
Contributor

abudd1094 commented Oct 30, 2023

https://tailwindcss.com/docs/hover-focus-and-other-states#form-states

Did someone try this in the components? (Not radio, as it's special.)
@mariohamann

:valid and :invalid work great... on an input element, not on the wrappers (which contain borders etc.)
I am now using const isInvalid = !this.checkValidity(); in the render method to conditionally style invalid inputs.

Many questions still open:

@solid-design-system/community

Should the input stretch to fill the width of its container by default?

Should clearable icon be visible if the user has not entered any text? In Shoelace, it is not, in your designs, it is.

What is the desired order of icons (for example when invalid and clearable)?:
image

Have we considered the "password" type? https://shoelace.style/components/input#toggle-password
image

There is nothing regarding passwords in the design. Shall I keep or take out the logic from Shoelace?

@coraliefeil
Copy link
Contributor

coraliefeil commented Oct 31, 2023

Should the input stretch to fill the width of its container by default?
-> I’d say yes.

Should clearable icon be visible if the user has not entered any text? In Shoelace, it is not, in your designs, it is.
-> No, clearable icon should only be visible if the user has entered some text AND if the application logic / UX concept wants it. Clearable should be an option and not visible by default.

What is the desired order of icons (for example when invalid and clearable)?:
-> Prefix-Icon – Text input – Clearable – Validation icons (error/success) – Suffix-Icon (@abudd1094 let’s have suffix icon on the far right -- cause this can be calendar icon or chevron-down or sth)

Have we considered the "password" type? https://shoelace.style/components/input#toggle-password
There is nothing regarding passwords in the design. Shall I keep or take out the logic from Shoelace?
-> No we haven’t. Need to discuss that with Brand. Can we comment it out for now and add it as a feature later?

@abudd1094
Copy link
Contributor

@coraliefeil thanks for your reply. I will implement. We can comment out password for now.

@abudd1094
Copy link
Contributor

@solid-design-system/design @solid-design-system/development
Shoelace includes the "date" type by default. In fact it provides all of the following:
` /**

  • The type of input. Works the same as a native <input> element, but only a subset of types are supported. Defaults
  • to text.
    */
    @Property({ reflect: true }) type:
    | 'date'
    | 'datetime-local'
    | 'email'
    | 'number'
    | 'password'
    | 'search'
    | 'tel'
    | 'text'
    | 'time'
    | 'url' = 'text';`
    image

Am I expected to keep these in the sd-input for use in future components such as a datepicker?

How extensive should the stories be? @mariohamann has only listed the following in the ticket requirements:
image

@abudd1094
Copy link
Contributor

@mariohamann
What's the difference between 'base' and 'form-control-input', do we really need both?
image
image

@mariohamann
Copy link
Contributor Author

It makes sense to have a form-control-input if we have a form-control-label and form-control-help-text as well. Having a base inside it is maybe more "standards" thing, as they have a base class everywhere. As they have a more semantic perspective as we with TailwindCSS I don't see a problem technical wise by removing e. g. the base. On your screenshot they apparently have different borders. But yeah, feel free to improve, if it makes sense from your perspective.

@mariohamann
Copy link
Contributor Author

Please let all text related types in it.

@mariohamann
Copy link
Contributor Author

Including number, date etc. @abudd1094 :)

@mariohamann mariohamann moved this from 🏗 In progress to 👀 In review in Solid Design System Project Board Nov 3, 2023
@abudd1094 abudd1094 mentioned this issue Nov 3, 2023
5 tasks
abudd1094 added a commit that referenced this issue Nov 13, 2023
Added sd-input component and samples. Closes #255
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in Solid Design System Project Board Nov 13, 2023
karlbaumhauer pushed a commit that referenced this issue Nov 13, 2023
# [@solid-design-system/components-v1.24.0](components/1.23.0...components/1.24.0) (2023-11-13)

### Features

* ✨ add sd-input ([#515](#515)) ([327dcd0](327dcd0)), closes [#255](#255)
@yoezlem yoezlem added the CL-migration All components which need to be migrated from Component Library label Jan 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CL-migration All components which need to be migrated from Component Library 🎨 figma needs changes in Figma
Projects
Development

Successfully merging a pull request may close this issue.

8 participants