2793 - Add validation indicator to IdsTab #2801

anhallbe · 2024-09-05T15:37:50Z

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

Adds an error indicator to IdsTab when its' IdsTabContent contains an invalid input.

Related github/jira issue (required): Fixes #2793

Summary:

Adds an error slot to IdsTab for customization (custom icon, tooltips, legacy IDS compat etc)
Adds a has-error attribute to IdsTab which can be used to display the slotted error.
Show the error in the tab title when there is a validation error inside the tab content.
The IdsValidateMixin's validate event now bubbles, and has stronger typings.

Related fixes:

Adds missing imports in IdsTab (icon & dismiss button)
Fix vertical alignment of dismiss button

Steps necessary to review your pull request (required):

Check out the branch
npm start
Go to http://localhost:4300/ids-tabs/validation.html
Go to every test/sample page that contains either form validation or tabs, and make sure they still work.

Included in this Pull Request:

Some documentation for the feature.
A test for the bug or feature.
A note to the change log.

src/components/ids-tabs/ids-tabs-context.ts

anhallbe · 2024-09-05T15:42:57Z

src/mixins/ids-validation-mixin/ids-validation-mixin.ts

@@ -199,7 +200,9 @@ const IdsValidationMixin = <T extends Constraints>(superclass: T) => class exten
        }
      });
      this.isTypeNotValid = null;
-      this.triggerEvent('validate', this, { detail: { elem: this, value: (this as IdsInputInterface).value, isValid } });
+      if (isValid !== wasValid) { // Only trigger if the state has changed, for performance.
+        this.#triggerValidateEvent(isValid);


This used to fire on every keypress. Added the isValid !== wasValid check for performance, but I'm not sure if this breaks anything.

This seems ok. But does it account for the case of clearing the field? i.e. reseting the state by clearing the text ect.

Like this?

Screen.Recording.2024-09-09.at.13.22.03.mov

src/mixins/ids-validation-mixin/ids-validation-mixin.ts

tmcconechy · 2024-09-05T17:18:33Z

Handle different validation error levels (error, warning, info)

Be nice if you did... Not required for me

Add an indicator to the "more" menu in case the invalid tab is overflowed

We didnt have this but i think if we did it would just be the icon next to the item.

Add an option to disable the error indicator?

Yes maybe DISABLE_VALIDATION. What was the VALIDATION_HAS_ERROR for? Just a flag?

Handle / test validation errors from ids-enterprise controls?

Not sure?

Right now this is implemented using the validation-has-error attribute. Not sure if this is the intended purpose of the attribute?

I dont know what that is for? Its only used on radio button. I think it can go. I dont see what its doing maybe just to add the class and show it has an error? Would use hasError instead maybe

The 'validated' event now bubbles so that parent elements can detect it (IdsTab, IdsModal, IdsForm etc). Not sure if there's a better/preferred way to do this?
I think this one makes sense to do it that way.

Test with more tab variants (module, counts etc).
Can push to later...

Needs docs / changeling

Please and maybe a test.

anhallbe · 2024-09-06T11:31:32Z

~~Needs a bit of work to align the icon for the dismissible, count and module cases (and all of these combined with orientation="vertical"...)~~

Update: This has now been fixed.

ericangeles · 2024-09-06T13:34:36Z

@anhallbe, have we considered the RTL style here?

ericangeles · 2024-09-06T13:40:33Z

tests/ids-tabs/ids-tabs.spec.ts

+        const input = content.locator('ids-input').first();
+        const icon = tab.locator('ids-icon[icon="error"]');
+
+        await input.evaluate((el: IdsInput) => { el.value = 'hello'; });


Maybe we can avoid repetition here by creating a function?

const setInputValue = async (value: string) => { await input.evaluate((el: IdsInput) => { el.value = value; }); };

src/components/ids-tabs/ids-tabs-context.ts

anhallbe · 2024-09-09T12:54:11Z

@anhallbe, have we considered the RTL style here?

anhallbe · 2024-09-09T13:01:27Z

Handle different validation error levels (error, warning, info)

Be nice if you did... Not required for me

Not implemented right now. But the icon is now slotted so people can implement that if they need to: cf59264

Add an indicator to the "more" menu in case the invalid tab is overflowed

We didnt have this but i think if we did it would just be the icon next to the item.

I guess for feature parity this can be saved for later, then?

Add an option to disable the error indicator?

Yes maybe DISABLE_VALIDATION. What was the VALIDATION_HAS_ERROR for? Just a flag?

Added a slot for the error, which can be hidden: cf59264

Handle / test validation errors from ids-enterprise controls?

Not sure?

Added a slot for the error, so this can be implemented by consumers in the meantime: cf59264

Right now this is implemented using the validation-has-error attribute. Not sure if this is the intended purpose of the attribute?

I dont know what that is for? It's only used on radio button. I think it can go. I dont see what it's doing maybe just to add the class and show it has an error? Would use hasError instead maybe

Switched to has-error.

Test with more tab variants (module, counts etc).
Can push to later...

@tmcconechy there appear to be an existing issue with the "dismissible" button (and this new error icon) being misaligned.

Needs docs / changeling

Please and maybe a test.

Added docs & test

tmcconechy · 2024-09-09T14:02:27Z

@anhallbe usually in that case just go ahead and include i fix with it (when i see issues in the same place I just correct them)

anhallbe · 2024-09-12T09:40:43Z

The alignment issue has been fixed, and I added a note in the Changelog (v 1.5.0). I think this is ready for review/test/merge.

Update: Oops. I just noticed that 1.5.0 has already been released. So I guess it'll be 1.6.0?

tmcconechy · 2024-09-12T12:25:19Z

@anhallbe This is great! Can you just check the tests https://github.com/infor-design/enterprise-wc/actions/runs/10826090266/job/30044182690?pr=2801#step:12:2667

Yep, This will be in 1.6.0 or 1.5.1 depending on semantic versioning as things change over the sprint. (Probably 1.6.0 considering whats in the Queue)

anhallbe · 2024-09-12T13:43:23Z

@tmcconechy the failing test was because of my change in when the validate event fired.

I had a change of heart regarding that one so I reverted the change in bd8edc9. The 'validate' event will again always fire when checkValidation() is called. This is consistent with how native elements always fire the invalid event when checkValidity() is called.

Also added a 1.6.0 section to the Changelog

- Add types to IdsValidationMixin & events - Use `has-error` instead of `validation-has-error` - Add method to IdsTabs for listing tabs, rather than querySelector from the outside.

…elements that have a 'validate' attribute

Use flex gap instead of inline padding to pad trigger button & error icon

checkValidation should always fire the "validate" event, similar to how the native checkValidity always fires an "invalid" event.

anhallbe commented Sep 5, 2024

View reviewed changes

src/components/ids-tabs/ids-tabs-context.ts Outdated Show resolved Hide resolved

anhallbe commented Sep 5, 2024

View reviewed changes

src/components/ids-tabs/ids-tabs-context.ts Outdated Show resolved Hide resolved

anhallbe commented Sep 5, 2024

View reviewed changes

src/components/ids-tabs/ids-tabs-context.ts Outdated Show resolved Hide resolved

anhallbe commented Sep 5, 2024

View reviewed changes

src/mixins/ids-validation-mixin/ids-validation-mixin.ts Show resolved Hide resolved

ericangeles reviewed Sep 6, 2024

View reviewed changes

src/components/ids-tabs/ids-tabs-context.ts Show resolved Hide resolved

tmcconechy added the status: wip 🚧 Work in Progress (Ignore for now) label Sep 6, 2024

anhallbe force-pushed the 2793-tabs-validation-indicator branch 2 times, most recently from 8d6559f to ccd046e Compare September 9, 2024 10:39

anhallbe closed this Sep 9, 2024

anhallbe reopened this Sep 9, 2024

anhallbe force-pushed the 2793-tabs-validation-indicator branch from cf59264 to 4365871 Compare September 12, 2024 06:48

anhallbe marked this pull request as ready for review September 12, 2024 07:23

anhallbe requested a review from a team as a code owner September 12, 2024 07:23

anhallbe added 6 commits September 12, 2024 15:43

Add validation indicator to IdsTab

921f712

Refactor tabs validation

e3fd932

- Add types to IdsValidationMixin & events - Use `has-error` instead of `validation-has-error` - Add method to IdsTabs for listing tabs, rather than querySelector from the outside.

Add more test cases for tabs

dd55707

Only check IdsValidationMixin elements for validity, rather than all …

3509e7b

…elements that have a 'validate' attribute

Add slot for tab error to allow cusomizing or hiding it

4bdb2a9

Add missing imports to tab component

2367a1b

anhallbe added 3 commits September 12, 2024 15:43

Vertically align tab text with icons

867c9b2

Use flex gap instead of inline padding to pad trigger button & error icon

Document Tab validation error in changelog

cf0668c

Revert validation optimization

bd8edc9

checkValidation should always fire the "validate" event, similar to how the native checkValidity always fires an "invalid" event.

anhallbe force-pushed the 2793-tabs-validation-indicator branch from 41533ba to bd8edc9 Compare September 12, 2024 13:44

tmcconechy removed the status: wip 🚧 Work in Progress (Ignore for now) label Sep 12, 2024

tmcconechy approved these changes Sep 12, 2024

View reviewed changes

tmcconechy merged commit ffd3a4a into infor-design:main Sep 12, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

2793 - Add validation indicator to IdsTab #2801

2793 - Add validation indicator to IdsTab #2801

anhallbe commented Sep 5, 2024 •

edited

Loading

anhallbe Sep 5, 2024

tmcconechy Sep 5, 2024

anhallbe Sep 9, 2024

tmcconechy commented Sep 5, 2024

anhallbe commented Sep 6, 2024 •

edited

Loading

ericangeles commented Sep 6, 2024

ericangeles Sep 6, 2024

anhallbe commented Sep 9, 2024 •

edited

Loading

anhallbe commented Sep 9, 2024

tmcconechy commented Sep 9, 2024

anhallbe commented Sep 12, 2024 •

edited

Loading

tmcconechy commented Sep 12, 2024

anhallbe commented Sep 12, 2024 •

edited

Loading

2793 - Add validation indicator to IdsTab #2801

2793 - Add validation indicator to IdsTab #2801

Conversation

anhallbe commented Sep 5, 2024 • edited Loading

anhallbe Sep 5, 2024

Choose a reason for hiding this comment

tmcconechy Sep 5, 2024

Choose a reason for hiding this comment

anhallbe Sep 9, 2024

Choose a reason for hiding this comment

tmcconechy commented Sep 5, 2024

anhallbe commented Sep 6, 2024 • edited Loading

ericangeles commented Sep 6, 2024

ericangeles Sep 6, 2024

Choose a reason for hiding this comment

anhallbe commented Sep 9, 2024 • edited Loading

anhallbe commented Sep 9, 2024

tmcconechy commented Sep 9, 2024

anhallbe commented Sep 12, 2024 • edited Loading

tmcconechy commented Sep 12, 2024

anhallbe commented Sep 12, 2024 • edited Loading

anhallbe commented Sep 5, 2024 •

edited

Loading

anhallbe commented Sep 6, 2024 •

edited

Loading

anhallbe commented Sep 9, 2024 •

edited

Loading

anhallbe commented Sep 12, 2024 •

edited

Loading

anhallbe commented Sep 12, 2024 •

edited

Loading