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/866-ensure-consistency-property-names #891

Merged
merged 13 commits into from
Feb 21, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -26,23 +26,21 @@ const sharedStyles = html`
const argTypesToDisable = [
'theme',
'variant',
'sizeVariant',
'icon',
'size',
'disabled',
'loading',
'readonly',
'required',
'buttonId',
'iconButtonId',
'hasError',
'errorMessage',
'errorIcon',
'arialabel',
'textareaId',
'name',
'onChange',
'onFocus',
'onBlur',
'onSelect',
];
const generateDisabledArgTypes = (argTypes: string[]) => {
const disabledArgTypes = {};
Expand Down Expand Up @@ -72,9 +70,8 @@ export default {
category: 'Appearance',
},
},
size: {
sizeVariant: {
options: ActionSizes,
name: 'sizeVariant',
description: 'Select size of the component.',
control: { type: 'select' },
table: {
Expand Down Expand Up @@ -123,37 +120,35 @@ export default {
},
},
//Technical attributes
buttonId: {
name: 'buttonId',
iconButtonId: {
description: 'Unique identifier for this component.',
table: {
category: 'Technical Attributes',
},
},

// Events
onChange: {
name: 'onChange',
description: 'Fires when the value changes.',
action: 'onChange',
table: {
disable: true,
category: 'Events',
},
},
onFocus: {
name: 'onFocus',
description: 'Fires when the component is focused.',
action: 'onFocus',
table: {
disable: true,
category: 'Events',
},
},
onBlur: {
name: 'onBlur',
description: 'Fires when the component lost focus.',
action: 'onBlur',
table: {
disable: true,
category: 'Events',
},
},
loadingStatus: {
Expand Down Expand Up @@ -195,12 +190,12 @@ BlrIconButton.storyName = 'Icon Button';
const defaultParams: BlrIconButtonType = {
theme: 'Light',
variant: 'primary',
size: 'md',
sizeVariant: 'md',
icon: 'blr360',
disabled: false,
loading: false,
arialabel: 'Icon Button',
buttonId: 'iconButtonId',
iconButtonId: 'iconButtonId',
loadingStatus: 'Loading',
};
BlrIconButton.args = defaultParams;
Expand Down Expand Up @@ -255,23 +250,23 @@ export const SizeVariant = () => {
<div class="stories-icon-button">
${BlrIconButtonRenderFunction({
...defaultParams,
size: 'xs',
sizeVariant: 'xs',
})}
${BlrIconButtonRenderFunction({
...defaultParams,
size: 'sm',
sizeVariant: 'sm',
})}
${BlrIconButtonRenderFunction({
...defaultParams,
size: 'md',
sizeVariant: 'md',
})}
${BlrIconButtonRenderFunction({
...defaultParams,
size: 'lg',
sizeVariant: 'lg',
})}
${BlrIconButtonRenderFunction({
...defaultParams,
size: 'xl',
sizeVariant: 'xl',
})}
</div>
`;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ const sampleParams: BlrIconButtonType = {
icon: 'blrChevronDown',
loading: false,
disabled: false,
buttonId: 'button-id',
iconButtonId: 'button-id',
variant: 'cta',
loadingStatus: 'Loading',
theme: 'Light',
Expand Down Expand Up @@ -55,7 +55,7 @@ describe('blr-icon-button', () => {
});

it('has a size sm when "size" is set to "sm" ', async () => {
const element = await fixture(BlrIconButtonRenderFunction({ ...sampleParams, size: 'sm' }));
const element = await fixture(BlrIconButtonRenderFunction({ ...sampleParams, sizeVariant: 'sm' }));

const iconButton = querySelectorDeep('.blr-icon-button', element.getRootNode() as HTMLElement);
const className = iconButton?.className;
Expand Down Expand Up @@ -107,13 +107,13 @@ describe('blr-icon-button', () => {
expect(className).not.to.contain('disabled');
});

it('fires blrclick event if clicked and not disabled', async () => {
it('fires onClick event if clicked and not disabled', async () => {
const element = await fixture(BlrIconButtonRenderFunction({ ...sampleParams, disabled: false }));

const button = querySelectorDeep('span', element.getRootNode() as HTMLElement);
let fired = false;

element.getRootNode()?.addEventListener('blrClick', () => {
element.getRootNode()?.addEventListener('onClick', () => {
fired = true;
});

Expand All @@ -122,13 +122,13 @@ describe('blr-icon-button', () => {
expect(fired).to.be.true;
});

it('doesnt fires blrclick event if clicked and disabled', async () => {
it('doesnt fires onClick event if clicked and disabled', async () => {
const element = await fixture(BlrIconButtonRenderFunction({ ...sampleParams, disabled: true }));

const button = querySelectorDeep('span', element.getRootNode() as HTMLElement);
let fired = false;

element.getRootNode()?.addEventListener('blrClick', () => {
element.getRootNode()?.addEventListener('onClick', () => {
fired = true;
});

Expand All @@ -137,13 +137,13 @@ describe('blr-icon-button', () => {
expect(fired).to.be.false;
});

it('fires blrfocus event if focused and not disabled', async () => {
it('fires onFocus event if focused and not disabled', async () => {
const element = await fixture(BlrIconButtonRenderFunction({ ...sampleParams, disabled: false }));

const button = querySelectorDeep('span', element.getRootNode() as HTMLElement);
let fired = false;

element.getRootNode()?.addEventListener('blrFocus', () => {
element.getRootNode()?.addEventListener('onFocus', () => {
fired = true;
});

Expand All @@ -152,13 +152,13 @@ describe('blr-icon-button', () => {
expect(fired).to.be.true;
});

it('doesnt fires blrfocus event if focused and disabled', async () => {
it('doesnt fires onFocus event if focused and disabled', async () => {
const element = await fixture(BlrIconButtonRenderFunction({ ...sampleParams, disabled: true }));

const button = querySelectorDeep('span', element.getRootNode() as HTMLElement);
let fired = false;

element.getRootNode()?.addEventListener('blrFocus', () => {
element.getRootNode()?.addEventListener('onFocus', () => {
fired = true;
});

Expand All @@ -167,13 +167,13 @@ describe('blr-icon-button', () => {
expect(fired).to.be.false;
});

it('fires blrblur event if blurred and not disabled', async () => {
it('fires onBlur event if blurred and not disabled', async () => {
const element = await fixture(BlrIconButtonRenderFunction({ ...sampleParams, disabled: false }));

const button = querySelectorDeep('span', element.getRootNode() as HTMLElement);
let fired = false;

element.getRootNode()?.addEventListener('blrBlur', () => {
element.getRootNode()?.addEventListener('onBlur', () => {
fired = true;
});

Expand All @@ -183,13 +183,13 @@ describe('blr-icon-button', () => {
expect(fired).to.be.true;
});

it('doesnt fires blrblur event if blurred and disabled', async () => {
it('doesnt fires onBlur event if blurred and disabled', async () => {
const element = await fixture(BlrIconButtonRenderFunction({ ...sampleParams, disabled: true }));

const button = querySelectorDeep('span', element.getRootNode() as HTMLElement);
let fired = false;

element.getRootNode()?.addEventListener('blrBlur', () => {
element.getRootNode()?.addEventListener('onBlur', () => {
fired = true;
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,25 +23,25 @@ export class BlrIconButton extends LitElement {
@property() icon?: SizelessIconType;
@property() loading?: boolean;
@property() disabled!: boolean;
@property() buttonId?: string;
@property() iconButtonId?: string;
@property() variant: ActionVariantType = 'primary';
@property() size?: ActionSizesType = 'md';
@property() sizeVariant?: ActionSizesType = 'md';
@property() loadingStatus!: string;

@property() theme: ThemeType = 'Light';

// these are not triggered directly but allows us to map it internally and bve typesafe
@property() blrFocus?: () => void;
@property() blrBlur?: () => void;
@property() blrClick?: () => void;
@property() onFocus?: () => void;
Copy link
Collaborator

Choose a reason for hiding this comment

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

we have to talk about this part :)

Yes, the ticket may suggest to do that, but when implementing the example app, we found out that it is highly recommended/needed to have a different event name, then standard html.

Because the inner html button will fire onBlur etc as well, it will bubble up and then you will end up having event listeners called twice :(

@property() onBlur?: () => void;
@property() onClick?: () => void;

@state() protected focused = false;

protected handleFocus = (event: Event) => {
if (!this.disabled) {
this.focused = true;
this.dispatchEvent(
new CustomEvent('blrFocus', { bubbles: true, composed: true, detail: { originalEvent: event } })
new CustomEvent('onFocus', { bubbles: true, composed: true, detail: { originalEvent: event } })
);
}
};
Expand All @@ -50,26 +50,26 @@ export class BlrIconButton extends LitElement {
if (!this.disabled) {
this.focused = false;
this.dispatchEvent(
new CustomEvent('blrBlur', { bubbles: true, composed: true, detail: { originalEvent: event } })
new CustomEvent('onBlur', { bubbles: true, composed: true, detail: { originalEvent: event } })
);
}
};

protected handleClick = (event: Event) => {
if (!this.disabled) {
this.dispatchEvent(
new CustomEvent('blrClick', { bubbles: true, composed: true, detail: { originalEvent: event } })
new CustomEvent('onClick', { bubbles: true, composed: true, detail: { originalEvent: event } })
);
}
};

protected render() {
if (this.size) {
if (this.sizeVariant) {
const dynamicStyles = this.theme === 'Light' ? [actionLight] : [actionDark];

const classes = classMap({
[this.variant]: this.variant,
[this.size]: this.size,
[this.sizeVariant]: this.sizeVariant,
disabled: this.disabled,
loading: this.loading || false,
});
Expand All @@ -83,15 +83,15 @@ export class BlrIconButton extends LitElement {
const loaderSizeVariant = getComponentConfigToken([
'SizeVariant',
'Actions',
this.size.toUpperCase(),
this.sizeVariant.toUpperCase(),
'Loader',
]).toLowerCase() as FormSizesType;

const iconSizeVariant = getComponentConfigToken([
'SizeVariant',
'Actions',
'IconButton',
this.size.toUpperCase(),
this.sizeVariant.toUpperCase(),
'Icon',
]).toLowerCase() as SizesType;

Expand All @@ -104,7 +104,7 @@ export class BlrIconButton extends LitElement {
class="blr-semantic-action blr-icon-button ${classes}"
aria-disabled=${this.disabled ? 'true' : nothing}
@click=${this.handleClick}
id=${this.buttonId || nothing}
id=${this.iconButtonId || nothing}
tabindex=${this.disabled ? nothing : '0'}
@focus=${this.handleFocus}
@blur=${this.handleBlur}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,13 +54,13 @@ export class BlrRangeLegendSlider extends LitElement {
protected renderBtn = ({ btnId, btnEventHandler, iconName }: RenderBtnProps) =>
html` ${BlrIconButtonRenderFunction({
arialabel: btnId,
blrClick: btnEventHandler,
onClick: btnEventHandler,
icon: iconName,
loading: false,
disabled: this.disabled || false,
buttonId: btnId,
iconButtonId: btnId,
variant: this.btnVariant,
size: this.size,
sizeVariant: this.size,
loadingStatus: 'Loading',
theme: this.theme,
})}`;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,13 +59,13 @@ export class BlrRangeSlider extends LitElement {
protected renderBtn = ({ btnId, btnEventHandler, iconName }: RenderBtnProps) =>
html` ${BlrIconButtonRenderFunction({
arialabel: btnId,
blrClick: btnEventHandler,
onClick: btnEventHandler,
icon: iconName,
loading: false,
disabled: this.disabled || false,
buttonId: btnId,
iconButtonId: btnId,
variant: this.btnVariant,
size: this.size,
sizeVariant: this.size,
loadingStatus: 'Loading',
theme: this.theme,
})}`;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,13 +56,13 @@ export class BlrRangeLegendMinMaxSlider extends LitElement {
protected renderBtn = ({ btnId, btnEventHandler, iconName }: RenderBtnProps) =>
BlrIconButtonRenderFunction({
arialabel: btnId,
blrClick: btnEventHandler,
onClick: btnEventHandler,
icon: iconName,
loading: false,
disabled: this.disabled || false,
buttonId: btnId,
iconButtonId: btnId,
variant: this.btnVariant,
size: this.size,
sizeVariant: this.size,
loadingStatus: 'Loading',
theme: this.theme,
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,13 +62,13 @@ export class BlrRangeMinMaxSlider extends LitElement {
protected renderBtn = ({ btnId, btnEventHandler, iconName }: RenderBtnProps) =>
html` ${BlrIconButtonRenderFunction({
arialabel: btnId,
blrClick: btnEventHandler,
onClick: btnEventHandler,
icon: iconName,
loading: false,
disabled: this.disabled || false,
buttonId: btnId,
iconButtonId: btnId,
variant: this.btnVariant,
size: this.size,
sizeVariant: this.size,
loadingStatus: 'Loading',
theme: this.theme,
})}`;
Expand Down
Loading