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(ui-library): align hint and error message icons #1059

Merged
merged 3 commits into from
Apr 2, 2024

Conversation

RubirajAccenture
Copy link
Contributor

@RubirajAccenture RubirajAccenture commented Mar 27, 2024

closes #515

@RubirajAccenture RubirajAccenture added the 🦹 needs:reviewers (code) The issue or pull request needs additional code reviewers label Mar 27, 2024
@@ -34,6 +34,23 @@ describe('blr-checkbox', () => {
expect(className).to.contain('input-control');
});

it('has error Icon is set to undefined', async () => {
Copy link
Contributor

@angsherpa456 angsherpa456 Mar 27, 2024

Choose a reason for hiding this comment

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

Please adjust it to it('has error Icon set to undefined'...) [remove 'is'] so that it will be consistency with others.

);

const formCaption = querySelectorDeep('.blr-form-caption', element?.getRootNode() as HTMLElement);

Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this extra new line.

BlrRadioGroupRenderFunction({
...sampleParams,
hasHint: false,
hasError: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

hasError needs to be true

BlrRadioRenderFunction({
...sampleParams,
hasHint: false,
hasError: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

hasError needs to be true

const labelWrapper = querySelectorDeep('.label-wrapper', element.getRootNode() as HTMLElement);
const captionWrapper = querySelectorDeep('.caption-wraper', labelWrapper?.getRootNode() as HTMLElement);
const formCaption = querySelectorDeep('.blr-form-caption', captionWrapper?.getRootNode() as HTMLElement);

Copy link
Contributor

Choose a reason for hiding this comment

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

This extra new line can be removed for the consistency purpose. Right now they are mixed up.

BlrTextareaRenderFunction({
...sampleParams,
hasHint: false,
hasError: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

hasError needs to be true


const textarea = querySelectorDeep('textarea', element.getRootNode() as HTMLElement);
const formCaption = querySelectorDeep('.blr-form-caption', textarea?.getRootNode() as HTMLElement);

Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this extra new line.

const formCaptions = querySelectorAllDeep('blr-form-caption', captionWrapper?.getRootNode() as HTMLElement);

const formCaptionError = querySelectorDeep('.blr-form-caption', formCaptions[1] as HTMLElement);
const errorIcon = formCaptionError?.getAttribute('errorIcon');
Copy link
Contributor

Choose a reason for hiding this comment

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

Please adjust this like other test and also adjust the line 51.

@thrbnhrtmnn thrbnhrtmnn removed the 🦹 needs:reviewers (code) The issue or pull request needs additional code reviewers label Mar 27, 2024
Copy link
Contributor

@thrbnhrtmnn thrbnhrtmnn left a comment

Choose a reason for hiding this comment

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

Hey @RubirajAccenture , I just found one small thing. If I set the Input Field Text component to "hasError"=true, I see a blrInfo icon. This should not be there, the default icon should be undefined. Please also check that the test that you implemented also works and finds this issue.

@RubirajAccenture RubirajAccenture force-pushed the align-hint-and-error-message-icons branch 2 times, most recently from 2df90fa to 5c13e91 Compare March 28, 2024 09:09
@RubirajAccenture RubirajAccenture force-pushed the align-hint-and-error-message-icons branch from 5c13e91 to 7908799 Compare March 28, 2024 09:18
Copy link
Contributor

@angsherpa456 angsherpa456 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

@thrbnhrtmnn thrbnhrtmnn left a comment

Choose a reason for hiding this comment

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

Approved

@thrbnhrtmnn thrbnhrtmnn merged commit 52929ca into develop Apr 2, 2024
4 of 5 checks passed
@thrbnhrtmnn thrbnhrtmnn deleted the align-hint-and-error-message-icons branch April 2, 2024 11:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Align how Hint & Error Messages are displayed in our component examples
5 participants