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 #515

Merged
merged 127 commits into from
Nov 13, 2023
Merged

feat: ✨ add sd-input #515

merged 127 commits into from
Nov 13, 2023

Conversation

abudd1094
Copy link
Contributor

@abudd1094 abudd1094 commented Nov 2, 2023

Description:

Added sd-input component and samples. Closes #255

Please read the TODO comments. They are meant to call attention to certain lines where there is not full clarity.

Definition of Reviewable:

  • Documentation has been created/updated (if applicable)
  • Migration Guide has been created/updated (if applicable)
  • Relevant E2E tests (Features, A11y, Bug fixes) are created/updated
  • Relevant stories (Features, A11y) are created/updated
  • Implementation works successfully on feature branch

yoezlem and others added 30 commits August 9, 2023 11:50
## Description:
Add css components sd-table and sd-table-cell.

## Definition of Reviewable:
- [x] Documentation is created/updated
- [x] Migration Guide is created/updated
- [x] Stories (features, a11y) are created/updated
- [x] relevant tickets are linked
- [x] PR is assigned to project board
# [@solid-design-system/components-v1.13.0](components/1.12.0...components/1.13.0) (2023-09-20)

### Features

* ✨ add sd-table and sd-table-cell ([#379](#379)) ([ef7c5a9](ef7c5a9))
# [@solid-design-system/components-v1.13.1](components/1.13.0...components/1.13.1) (2023-09-21)

### Bug Fixes

* 🤔changed paragraph default size to text-base ([#419](#419)) ([40e6605](40e6605))
Author: yoezlem <[email protected]>
Co-authored-by: mariohamann <[email protected]>
# [@solid-design-system/components-v1.14.0](components/1.13.1...components/1.14.0) (2023-09-21)

### Features

* ✨ quote pattern ([#405](#405)) ([f8bc6d8](f8bc6d8))
…cted in sd-teaser (#422)

## Description:
Fixed a bug in sd-teaser to prevent rewrite of inset property when
changing variants and added a corresponding test for it. Closes #362

## Definition of Reviewable:
*PR notes: Irrelevant elements should be removed.*
- [x] relevant tickets are linked
- [x] PR is assigned to project board
# [@solid-design-system/components-v1.14.1](components/1.14.0...components/1.14.1) (2023-09-22)

### Bug Fixes

* 🐛 prevent rewrite of inset property when border variant was selected in sd-teaser ([#422](#422)) ([ada5be7](ada5be7)), closes [#362](#362)
This reverts commit 9ab2401.
# [@solid-design-system/components-v1.14.2](components/1.14.1...components/1.14.2) (2023-09-25)

### Bug Fixes

* 🐛 fix CEM loading in Storybook ([#427](#427)) ([de7368f](de7368f))
# [@solid-design-system/components-v1.14.3](components/1.14.2...components/1.14.3) (2023-09-25)

### Bug Fixes

* 🤔 make styles available on CDN ([#411](#411)) ([8048e94](8048e94))
# [@solid-design-system/components-v1.14.4](components/1.14.3...components/1.14.4) (2023-09-25)

### Bug Fixes

* use own customElement decorator ([6a2787f](6a2787f))
@coraliefeil
Copy link
Contributor

Pls reassign when last comments have been implemented. THX

@coraliefeil coraliefeil removed their assignment Nov 9, 2023
@coraliefeil
Copy link
Contributor

coraliefeil commented Nov 9, 2023

Approved. Thank you @abudd1094 for pushing through! Really a tough component. Appreciate all your helpful questions. <3

@coraliefeil coraliefeil removed their assignment Nov 9, 2023
@karlbaumhauer
Copy link
Contributor

As there have been so many reviews already, I will remove myself from the assignees. I dont feel, I could contribute anything meaningful to the PR and especially dont want to block the merge anymore.

If you need anything specific from me about this PR, pls reach out to me directly.

@karlbaumhauer karlbaumhauer removed their assignment Nov 10, 2023
@yoezlem
Copy link
Contributor

yoezlem commented Nov 10, 2023

Ok, more updates are pushed.

  1. help-text is now text-neutral-700 for all states and variants. This is not documented in Figma, refer to comment from Coralie on Chromatic:
    image
  2. As per the discussion, text-neutral-400 is not available. text-neutral-500 is used instead, TODO comment left:
    image
  3. Do we want to leave the 'message' slot in the component or remove it?
    image
  4. Solid icon styling. For the calendar and clock, this is an open question. Is the order correct and how would you like to apply colors in the various states to these icons @coraliefeil? Currently nothing is implemented so it inherits color.
    image
  5. Regarding differences in the input types, I have followed Mario's and Coralie's suggestions. Solid clock icon is used for the time type. Solid calendar icon is used for the date and datetime-local types, except for firefox, which keeps the default browser datepicker instead (impossible to hide without ugly hacks). flatpickr will not allow someone to use sd-input with type 'date' or 'datetime-local' it will foce the input to 'text' type and thus voids the built in browser formatting and validation. This is an open topic for us and how we would like to handle form validation and dates in our library.
  6. As far as a 'success' state for validation, I'm not sure if my implementation is what is expected. Please have a look. @saemik94 made a valid point above that he did not understand the conditions for 'hasValidationAttr', 'isInvalid', and 'isValid' which I used in the render method. Is it also confusing for you? How would you recommend changing it if so?
  7. I updated the stories to resemble Shoelace, they have a pretty readable presentation. The sample is also fixed. I think it can be refined a bit further. Maybe we should replace the Validation story with a types story? Any thoughts?

I await new feedback. @solid-design-system/current-sds-team

  1. @coraliefeil do you need to document this to Figma too?
  2. Thx for the comment, do we need to create also a follow up ticket for that? @mariohamann
  3. I would leave it as it is, but we can also create a follow-up ticket (as I mentioned in Point 2), remove it here for now, and specify in the follow-up ticket that it needs to be added back. However, I find it unnecessary, so I suggest we leave it in.
  4. For me it looks how it should!
  5. I'm not sure if we should postpone this discussion until we have a "proper" datepicker. Documentation will likely be crucial there so that the user understands what they can do with and Flatpickr .
  6. I find it understandable, and I'm not sure how it could be explained in more detail.
  7. can't we do both?

@coraliefeil
Copy link
Contributor

  1. All adjustments while coding are documented in the figma branch
  2. I opened up a ticket tackling color tokens icon-fill / text - foreground #155
  3. – not design related
  4. Fine ...
  5. We’ll talk next week. Postponing sounds good.
  6. We could add an inline notification or just a text on top telling the user to fill out all required fields to see valid states

@DanielHargesheimer DanielHargesheimer removed their assignment Nov 10, 2023
Copy link
Contributor

@mariohamann mariohamann left a comment

Choose a reason for hiding this comment

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

From what I see from code there are just some small issues to resolve. If I missed a comment or something where you need feedback, please give ma hint via chat. :) (Will have a look on the comments in the PR now.)

packages/components/src/components/input/input.ts Outdated Show resolved Hide resolved
packages/components/src/components/input/input.ts Outdated Show resolved Hide resolved
packages/components/src/components/input/input.ts Outdated Show resolved Hide resolved
packages/components/src/components/input/input.ts Outdated Show resolved Hide resolved
packages/components/src/components/input/input.ts Outdated Show resolved Hide resolved
packages/components/src/components/input/input.ts Outdated Show resolved Hide resolved
packages/components/src/components/input/input.ts Outdated Show resolved Hide resolved
packages/components/src/components/input/input.ts Outdated Show resolved Hide resolved
packages/components/src/components/input/input.stories.ts Outdated Show resolved Hide resolved
@mariohamann
Copy link
Contributor

mariohamann commented Nov 13, 2023

Okay, from what I read everything from a concept perspective is resolved. So if you can implemented the suggested changes I think we're good to go!

Props for the stories. This is really extensive and helpful.

@mariohamann mariohamann removed their assignment Nov 13, 2023
@mariohamann
Copy link
Contributor

Oh, one last thing before I forget it: Please add transitions to the colors especially on hover, but maybe even focus and error. We try to avoid hard color changes.

@abudd1094
Copy link
Contributor Author

Oh, one last thing before I forget it: Please add transitions to the colors especially on hover, but maybe even focus and error. We try to avoid hard color changes.

I've added transition-all to the base and border parts, adding it to input yields some strange behaviors (flickering of border around native input). Give a shout if it is still lacking

Copy link
Contributor

@mariohamann mariohamann left a comment

Choose a reason for hiding this comment

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

Really appreciate your effort here. 🙏🏻

@abudd1094 abudd1094 merged commit 327dcd0 into main Nov 13, 2023
8 checks passed
@abudd1094 abudd1094 deleted the feat/sd-input branch November 13, 2023 14:39
karlbaumhauer pushed a commit that referenced this pull request 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 🔧 code needs changes in code CL-migration All components which need to be migrated from Component Library labels 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 🔧 code needs changes in code
Projects
Development

Successfully merging this pull request may close these issues.

feat: ✨ add sd-input