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

139 - ids-spinbox wc #206

Closed
wants to merge 53 commits into from
Closed

139 - ids-spinbox wc #206

wants to merge 53 commits into from

Conversation

rob2d
Copy link
Contributor

@rob2d rob2d commented Jun 1, 2021

Explain the details for making this change. What existing problem does the pull request solve?

Adds IdsSpinbox (Phase 1) and base attributes.

Misc: also fixes:

a small potential issue with ids-keyboard-mixin where we sub on an array as the key vs the events (since we do not actually group key combos or use it in this manner in other components -- the fix was 2 lines so figured small enough to include here)

Related github/jira issue (required):

Closes #139

Steps necessary to review your pull request (required):

pull this PR branch locally and run environment (139-ids-spinbox)
visit http://localhost:4300/ids-spinbox/
ensure functionality listed at #139 for Phase 1 is implemented.

Included in this Pull Request:

  • An e2e or functional test for the bug or feature.
  • A note to the change log.

rob2d added 30 commits June 1, 2021 11:24
@rob2d rob2d requested a review from a team as a code owner June 1, 2021 15:31
@rob2d rob2d closed this Jun 1, 2021
@rob2d rob2d reopened this Jun 1, 2021
Copy link
Member

@tmcconechy tmcconechy left a comment

Choose a reason for hiding this comment

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

Looks really good and getting more consistent. Just a couple things that could be improved (some could be moved to TODO).

  1. Can you make sure it works in all themes see https://github.com/infor-design/enterprise-wc/tree/main/src/ids-mixins#ids-theme-mixin for details (all the components you did are missing this so should get in the habit)
  2. The focus state is looking a bit strange
    Screen Shot 2021-06-02 at 11 07 19 AM
  3. We have a feature i just remembered where you can hold and press the spinbox

My suggestion would be fix 1 and 2 and add 3 to the TODO for now but feel free to add

@@ -0,0 +1,2 @@
import IdsSpinbox from '../../src/ids-spinbox';
import './demo.scss';
Copy link
Member

Choose a reason for hiding this comment

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

This can go in example.js (or else the demo css will go in the built component)

Copy link
Contributor Author

@rob2d rob2d Jun 2, 2021

Choose a reason for hiding this comment

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

not sure if this is user error here, but tried to put this into app/ids-spinbox/example.js and it doesn't actually reflect when visiting the main app index.

Copy link
Member

Choose a reason for hiding this comment

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

I did it here https://github.com/infor-design/enterprise-wc/tree/main/app/ids-tooltip but not sure about the main index either. You maybe need to import it index.js but sounds like this is for layout? If so i would just remove it and use ids-layout grid?

Copy link
Contributor

@deep7102 deep7102 left a comment

Choose a reason for hiding this comment

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

@rob2d looking very nice, few things found

  • Why not used the use ids-layout-grid for demo example
  • Missing read-only example
  • Initial load focus was on last spin-input
  • Input is fine, but each button around looks bit wider
  • Type in 10 then click on ant button +/- value got reset
  • In 0-to-5 spin box, it let enter other values like 60
  • Missing getter/setter for label, label-required, readonly and placeholder props
  • Do we need label-required in demo example? by default it should be true

app/ids-spinbox/demo.scss Outdated Show resolved Hide resolved
app/ids-spinbox/example.html Outdated Show resolved Hide resolved
app/ids-spinbox/example.html Outdated Show resolved Hide resolved
@rob2d
Copy link
Contributor Author

rob2d commented Jun 2, 2021

@deep7102 thanks for the feedback.

Re: Type in 10 then click on ant button +/- value got reset

I saw in the issue it said handling blur/etc was not for Phase I, so I was aware of that but just pushing with functionality as it is since that's a related edge case.

Edit: seems for other issues I do at least need a change handler here.

@rob2d rob2d added skip ci tests Do not run a build or tests on CI status: wip 🚧 Work in Progress (Ignore for now) labels Jun 2, 2021
@rob2d rob2d closed this Jun 2, 2021
@rob2d
Copy link
Contributor Author

rob2d commented Jun 3, 2021

@deep7102 seems placeholder and label setters are there

for

Initial load focus was on last spin-input

I believe this is a separate issue/slightly out of scope and on ids-input at the moment since I'm not adding code for managing focus state beyond when user has retrieved it. Can see this when visiting http://localhost:4300 (document.activeState is on "active focus" component but that shouldn't be auto binding)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip ci tests Do not run a build or tests on CI status: wip 🚧 Work in Progress (Ignore for now)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IdsSpinBox: Add new web component
3 participants