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

fix(text-field): read-only hides suffix icon #554

Merged
merged 4 commits into from
Mar 19, 2024

Conversation

theJohnnyMe
Copy link
Contributor

@theJohnnyMe theJohnnyMe commented Mar 18, 2024

Describe pull-request
When using the suffix icon and read-only, the read-only icon takes priority, and the suffix icon remains hidden.

Solving issue
Fixes: CDEP-3182
#522

How to test

  1. Add suffix icon
  2. Turn on "read-only"
  3. Suffx icon should be hidden
  4. PS. Please check that other controls like size, mode-variant, and state are working too. I had to change from using string literals to using an object way of writing CSS as string literals had some hiccups when changing values in runtime.

Checklist before submission

  • I have added unit tests for my changes (if applicable)
  • All existing tests pass
  • I have updated the documentation (if applicable)

Suggested test steps

  • Browser testing (Chrome, Safari, Firefox)
  • Keyboard operability
  • Interactive elements have labels.
  • Storybook controls
  • Design/controls/props is aligned with other components
  • Dark/light mode and variants
  • Input fields – values should be displayed properly
  • Events

@theJohnnyMe theJohnnyMe added 🍾 improvement 🪆 refactor Changes implementation but not function labels Mar 18, 2024
@theJohnnyMe theJohnnyMe self-assigned this Mar 18, 2024
Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-554.d3fazya28914g3.amplifyapp.com

@theJohnnyMe theJohnnyMe requested a review from mJarsater March 19, 2024 08:11
@theJohnnyMe theJohnnyMe force-pushed the fix/CDEP-3182-text-fiel-readonly-icon branch from cbd35ab to dd22353 Compare March 19, 2024 08:11
Copy link
Contributor

@timrombergjakobsson timrombergjakobsson left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@nathalielindqvist nathalielindqvist left a comment

Choose a reason for hiding this comment

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

Don't know if this should be part of this PR, but it looks like the suffix icon and the read-only icon have different margins/positioning when the components it put in Medium or Small size. Should that be handled somehow?
Medium:
image
image

Small:
image
image

@theJohnnyMe
Copy link
Contributor Author

Don't know if this should be part of this PR, but it looks like the suffix icon and the read-only icon have different margins/positioning when the components it put in Medium or Small size. Should that be handled somehow? Medium: image image

Small: image image
Nice finding @nathalielindqvist - Is it the case in prod version too or only in this PR ?

@theJohnnyMe
Copy link
Contributor Author

Seems like it is in prod too @nathalielindqvist. I will check it and if it is simple (it seems), I will fix it in this PR ;) Nice finding really ;)

@theJohnnyMe
Copy link
Contributor Author

@nathalielindqvist - Change pushed! ;)

Copy link

Quality Gate Passed Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link
Contributor

@nathalielindqvist nathalielindqvist left a comment

Choose a reason for hiding this comment

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

Looks great! ⭐

@theJohnnyMe theJohnnyMe merged commit 79e5340 into develop Mar 19, 2024
2 checks passed
@theJohnnyMe theJohnnyMe deleted the fix/CDEP-3182-text-fiel-readonly-icon branch March 19, 2024 14:42
timrombergjakobsson added a commit that referenced this pull request Mar 21, 2024
* Release of @scania/tegel@1.6.1 (#525)

Co-authored-by: theJohnnyMe <theJohnnyMe@users.noreply.github.com>

* feat(date-picker): inital date-picker development (#375)

* feat(date picker): inital date picker development

* feat(date picker): added custom weekday label support

* fix(date picker): added key for itirator

* fix(date picker): enable custom date selection

* feat(date picker): label, labelPosition and state props added

* feat(date picker): added basic language func

* feat(date picker): updated calender view if previous or next month date was chosen

* feat(range): added a range variant

* fix(date-range-picker): removed variant prop

* fix(date picker): fixes

* fix(date picker): corrected picking date in upcoming/previous months

* feat(date): corrected look of "today" date

* fix(fixes): fixes

* fix(date range picker): cleanup

* refactor(date picker): refactored and commented code

* refactor(date picker): refactored and slimmed down amount of css variables

* refactor(date range picker): removed vars file specific to range

* refactor(date range picker): commented and refactored code

* docs(date picker): added max control to storybook

* fix(date range picker): minor cleanup

* fix(date picker): added start and end helper/labels

* fix(date picker): aligned value with native date picker attribute

* fix(date picker): cleanup

* fix(date picker): cleanup

* fix(date picker): updated month format

* fix(date picker): introduces weekStartsOn property with examples

* docs(date picker): updated readme

* docs(date picker): updated readmes

* feat(date range picker): added weekStartsOn property

* fix(date  picker): minor accessability improvement

* fix(date range picker): minor accessability improvements

* feat(date picker): corrected the placeholder

* fix(date picker): utilize date-time attribute

* fix(date range picker): utilize the date-time attribute

* Removed variant from date-range-picker story args

* build: dev/production separation

* build: different storybook and build rules for dev and prod environment

* chore: clean up

---------

Co-authored-by: theJohnnyMe <nikola.johnny.mirkovic@scania.com>

* build(deps-dev): bump ip from 2.0.0 to 2.0.1 in /packages/core (#519)

Bumps [ip](/~https://github.com/indutny/node-ip) from 2.0.0 to 2.0.1.
- [Commits](indutny/node-ip@v2.0.0...v2.0.1)

---
updated-dependencies:
- dependency-name: ip
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* build(deps-dev): bump ip from 2.0.0 to 2.0.1 in /packages/angular (#520)

Bumps [ip](/~https://github.com/indutny/node-ip) from 2.0.0 to 2.0.1.
- [Commits](indutny/node-ip@v2.0.0...v2.0.1)

---
updated-dependencies:
- dependency-name: ip
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* build(deps-dev): bump ip from 1.1.8 to 1.1.9 in /icons (#523)

Bumps [ip](/~https://github.com/indutny/node-ip) from 1.1.8 to 1.1.9.
- [Commits](indutny/node-ip@v1.1.8...v1.1.9)

---
updated-dependencies:
- dependency-name: ip
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* test(checkbox): added unit tests (#512)

* test(checkbox): adding test for disabled and indeterminate state

* test(checkbox): update scenario

* test: snapshot update

* test(checkbox): updating test as per review

* test(checkbox): target label correctly

* style: adding label test

---------

Co-authored-by: theJohnnyMe <nikola.johnny.mirkovic@scania.com>

* fix(slider): emit tdsChange on mouse-up and touch-end events (#513)

* fix(slider): emit tdsChange on mouse-up and touch-ed events

* fix(slider): tdsInput event

* fix: drop conts in story

* fix(slider): avoid double events + ux improvement

* refactor: do not repeat yourself

* chore: docs update

* docs: update to conventions and contribution (#532)

* docs: update to conventions and contribution

* Update .github/CODE_STYLE.md

Co-authored-by: mJarsater <62651103+mJarsater@users.noreply.github.com>

* docs: review feedback

---------

Co-authored-by: mJarsater <62651103+mJarsater@users.noreply.github.com>

* ci: main as reference (#540)

* ci: use release branch (#542)

* ci: use release branch

* ci: fix

* test(chip): added unit tests (#537)

* feat(chip): initial default unit test

* test(chip): update default test

* test(chip): update default test

* test(chip): add initial test for chip type radio

* test(chip): add type radio button test

* test(chip): add tests for type radio

* test(chip): add tests for type radio

* test(chip): add tests for type checkbox

* test(chip): add tests for chip with icon

* test(chip): update test for chip with icon

* fix(toast): remove content ellipsis and add margin to close button (#538)

* feat(toast): make close button optional (#550)

* feat(toast): make close button optional

* fix(toast): update word break rule

* ci: release all script improvement (#544)

* feat(modal): re-initilize selecor and clean-up method (#552)

* build(stencil): v4.12.6 (#553)

* build(deps-dev): bump follow-redirects in /packages/angular (#551)

Bumps [follow-redirects](/~https://github.com/follow-redirects/follow-redirects) from 1.15.4 to 1.15.6.
- [Release notes](/~https://github.com/follow-redirects/follow-redirects/releases)
- [Commits](follow-redirects/follow-redirects@v1.15.4...v1.15.6)

---
updated-dependencies:
- dependency-name: follow-redirects
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* fix(text-field): read-only hides suffix icon (#554)

* fix(text-field): read-only hides suffix icon

* test: adding a test case for read-only & suffix

* docs(text-filed): read-only & suffix description

* fix(text-field): read-only icon position

* fix(table-body-cell): Behavior and additional `text-align` values in `tds-header-cell` and `tds-body-cell` (#555)

* feat: improved and added additional `center` text alignment feature to `tds-header-cell` component

* feat: improved and added additional `center` text alignment feature to `tds-body-cell` component

* feat: improved `text-align` behavior

* test: added controls in StoryBook

* test: added controls in StoryBook

* test: added controls in StoryBook

* fix: added default values for `tds-body-cell`

* Update packages/core/src/components/table/table-header-cell/table-header-cell.tsx

Co-authored-by: theJohnnyMe <johnny.bradata.kurva@gmail.com>

* Update packages/core/src/components/table/table-component-basic.stories.tsx

Co-authored-by: theJohnnyMe <johnny.bradata.kurva@gmail.com>

* fix: minor alingment & standardization

* fix: text alignment in sortable header cell

---------

Co-authored-by: theJohnnyMe <johnny.bradata.kurva@gmail.com>
Co-authored-by: theJohnnyMe <nikola.johnny.mirkovic@scania.com>

* test(text-field): default test case (#558)

* test(table-expandable): increae tolerance in screenshots

* test(text-field): default test case

* test(table-filtering): increase pixel tolerance

* Release of @scania/tegel@1.7.0 (#560)

Co-authored-by: timrombergjakobsson <timrombergjakobsson@users.noreply.github.com>

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: theJohnnyMe <theJohnnyMe@users.noreply.github.com>
Co-authored-by: mJarsater <62651103+mJarsater@users.noreply.github.com>
Co-authored-by: theJohnnyMe <nikola.johnny.mirkovic@scania.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Nathalie Lindqvist <nathalie.lindqvist@scania.com>
Co-authored-by: Andreas Rasovic <163417029+AndreasRasovicScania@users.noreply.github.com>
Co-authored-by: theJohnnyMe <johnny.bradata.kurva@gmail.com>
Co-authored-by: timrombergjakobsson <timrombergjakobsson@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🍾 improvement 🪆 refactor Changes implementation but not function
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants