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.

Sign up for GitHub

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

Feature/sc 109316/fe pro dashboard accessibility fixes #899

Merged
merged 7 commits into from
Jan 29, 2024
Merged

Feature/sc 109316/fe pro dashboard accessibility fixes #899

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3f59c2f
FE Pro Dashboard Accessibility fixes [sc-109316]
meetup-oleksandr-dudiuk Jan 26, 2024
5f07efa
FE Pro Dashboard Accessibility fixes [sc-109316]
meetup-oleksandr-dudiuk Jan 26, 2024
99c6026
some changes
meetup-oleksandr-dudiuk Jan 26, 2024
d717182
fix toggle
meetup-oleksandr-dudiuk Jan 26, 2024
463695a
refactor
meetup-oleksandr-dudiuk Jan 29, 2024
11373c9
refactor
meetup-oleksandr-dudiuk Jan 29, 2024
2ca74a6
refactor
meetup-oleksandr-dudiuk Jan 29, 2024
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
12 changes: 12 additions & 0 deletions assets/css/main.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3476,6 +3476,18 @@ $('#fadeInLink').on('click', function(e) {
background-color: #e63a6a !important;
border: 1px solid #e63a6a !important; }

/* with icon */
[data-swarm-button][data-icon="left"] .svg-icon,
[data-swarm-button][data-icon="left"] [data-swarm-icon], [data-swarm-link][data-icon="left"] .svg-icon {
margin-right: var(--space-half); }

[data-swarm-button][data-icon="right"] .svg-icon,
[data-swarm-button][data-icon="right"] [data-swarm-icon], [data-swarm-link][data-icon="right"] .svg-icon {
margin-left: var(--space-half); }

[data-swarm-link][data-icon="only"] {
padding: var(--space-half); }

/*doc
---
title: Cards
Expand Down
33 changes: 33 additions & 0 deletions assets/scss/components/_button.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,3 +239,36 @@
border: 1px solid $active !important;
}
}

/* with icon */
%swarm-button-icon-left {
margin-right: var(--space-half);
}
%swarm-button-icon-right {
margin-left: var(--space-half);
}
%swarm-button-icon-only {
padding: var(--space-half);
}

[data-swarm-button][data-icon="left"] .svg-icon,
[data-swarm-button][data-icon="left"] [data-swarm-icon] {
@extend %swarm-button-icon-left;
}

[data-swarm-button][data-icon="right"] .svg-icon,
[data-swarm-button][data-icon="right"] [data-swarm-icon] {
@extend %swarm-button-icon-right;
}

[data-swarm-link][data-icon="left"] .svg-icon {
@extend %swarm-button-icon-left;
}

[data-swarm-link][data-icon="right"] .svg-icon {
@extend %swarm-button-icon-right;
}

[data-swarm-link][data-icon="only"] {
@extend %swarm-button-icon-only;
}
2 changes: 1 addition & 1 deletion src/__snapshots__/footer.test.jsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ exports[`Footer exists 1`] = `
items={
Array [
<span>
© 2023 Meetup LLC
© 2024 Meetup LLC
</span>,
]
}
Expand Down
46 changes: 42 additions & 4 deletions src/forms/Button.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import PropTypes from 'prop-types';
import React from 'react';
import Icon from '../media/Icon';

import { Button as SwarmButton, LinkButton as SwarmLink } from '@meetup/swarm-components';
import { LinkButton as SwarmLink } from '@meetup/swarm-components';

import {
getButtonType,
Expand All @@ -26,6 +26,11 @@ export class Button extends React.PureComponent {
icon,
hasHoverShadow, // eslint-disable-line no-unused-vars
to,
children,
iconOnly,
forwardedRef,
right,
iconProps,
...other
} = this.props;

Expand Down Expand Up @@ -68,11 +73,11 @@ export class Button extends React.PureComponent {
right ? (
<React.Fragment>
{children}
<Icon shape={iconShape} size="xs" />
<Icon shape={iconShape} size="xs" {...iconProps} />
</React.Fragment>
) : (
<React.Fragment>
<Icon shape={iconShape} size="xs" />
<Icon shape={iconShape} size="xs" {...iconProps} />
{children}
</React.Fragment>
)
Expand All @@ -85,7 +90,40 @@ export class Button extends React.PureComponent {
return <SwarmLink icon={iconShape} grow={fullWidth} {...other} />;
}

return <SwarmButton icon={iconShape} grow={fullWidth} {...other} />;
const buttonType = getButtonType(this.props);
const width = fullWidth ? 'grow' : 'default';

return (
<button
data-swarm-button={buttonType}
data-swarm-size={getSwarmSize(this.props)}
data-icon={getIconPosition(this.props)}
data-swarm-width={width}
type="button"
ref={forwardedRef}
{...other}
>
{icon ? (
Copy link
Contributor

Choose a reason for hiding this comment

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

It's really hard to read multiple ternary operators in jsx and I surprised that it is allowed in eslint rules. Can we move it to a separate render function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's just a copypast from swarm repo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we have the same ternary above ) from 2019

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's a good chance to improve the code ;)

<span>
{right ? (
<React.Fragment>
{children}
<Icon shape={iconShape} size="xs" {...iconProps} />
</React.Fragment>
) : (
<React.Fragment>
<Icon shape={iconShape} size="xs" {...iconProps} />
{children}
</React.Fragment>
)}
</span>
) : iconOnly ? (
<span>{children}</span>
) : (
children
)}
</button>
);
}
}

Expand Down
7 changes: 7 additions & 0 deletions src/forms/ToggleSwitch.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ class ToggleSwitch extends React.Component {
id,
name,
disabled,
ariaLabel,
...other
} = this.props;

Expand All @@ -40,6 +41,12 @@ class ToggleSwitch extends React.Component {
aria-checked={checked}
aria-readonly={disabled}
disabled={disabled}
aria-label={
ariaLabel ||
(typeof label === 'string'
? `${label} - toggle switch`
: 'toggle switch')
}
{...labelProps}
{...other}
>
Expand Down
10 changes: 9 additions & 1 deletion src/forms/__snapshots__/button.test.jsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,11 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`Button is a HTML button element exists 1`] = `<Button />`;
exports[`Button is a HTML button element exists 1`] = `
<button
data-icon="only"
data-swarm-button="default"
data-swarm-size="default"
data-swarm-width="default"
type="button"
/>
`;
2 changes: 2 additions & 0 deletions src/interactive/__snapshots__/accordionPanel.test.jsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,7 @@ exports[`AccordionPanel Locked panel with toggle switch renders locked toggle pa
<div>
<button
aria-checked={false}
aria-label="toggle switch"
aria-labelledby="label-firstsection"
aria-readonly={false}
className="toggleSwitch"
Expand Down Expand Up @@ -703,6 +704,7 @@ exports[`AccordionPanel Panel with toggle switch exists and renders a toggle swi
<div>
<button
aria-checked={false}
aria-label="toggle switch"
aria-labelledby="label-firstsection"
aria-readonly={false}
className="toggleSwitch"
Expand Down
113 changes: 56 additions & 57 deletions src/interactive/__snapshots__/tooltip.test.jsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,19 +78,16 @@ exports[`Tooltip tooltip with a close button should render correctly 1`] = `
<Button
small={true}
>
<Button
<button
data-icon="left"
data-swarm-button="default"
data-swarm-size="small"
data-swarm-width="default"
small={true}
type="button"
>
<button
data-icon="left"
data-swarm-button="default"
data-swarm-size="small"
data-swarm-width="default"
type="button"
>
Open
</button>
</Button>
Open
</button>
</Button>
</Button>
</div>
Expand Down Expand Up @@ -140,18 +137,20 @@ exports[`Tooltip tooltip with a close button should render correctly 1`] = `
type="button"
>
<span>
<svg
class="svg svg--cross svg-icon valign--middle"
height="20"
preserveAspectRatio="xMinYMin meet"
role="img"
viewBox="0 0 20 20"
width="20"
>
<use
xlink:href="#icon-cross--small"
/>
</svg>
<span>
<svg
class="svg svg--cross svg-icon valign--middle"
height="20"
preserveAspectRatio="xMinYMin meet"
role="presentation"
viewBox="0 0 20 20"
width="20"
>
<use
xlink:href="#icon-cross--small"
/>
</svg>
</span>
</span>
</button>
<section
Expand Down Expand Up @@ -217,47 +216,47 @@ exports[`Tooltip tooltip with a close button should render correctly 1`] = `
reset={true}
role="button"
>
<Button
<button
aria-label="Close"
className="tooltip-closeBtn"
icon="cross"
data-icon="only"
data-swarm-button="reset"
data-swarm-size="default"
data-swarm-width="default"
onClick={[Function]}
reset={true}
role="button"
type="button"
>
<button
aria-label="Close"
className="tooltip-closeBtn"
data-icon="only"
data-swarm-button="reset"
data-swarm-size="default"
data-swarm-width="default"
onClick={[Function]}
role="button"
type="button"
>
<span>
<Icon
shape="cross"
size="xs"
>
<svg
className="svg svg--cross svg-icon valign--middle"
height="20"
preserveAspectRatio="xMinYMin meet"
role="img"
style={Object {}}
viewBox="0 0 20 20"
width="20"
<span>
<Icon
shape="cross"
size="xs"
>
<span>
<Icon
role="presentation"
shape="cross"
size="xs"
>
<use
xlinkHref="#icon-cross--small"
/>
</svg>
</Icon>
</span>
</button>
</Button>
<svg
className="svg svg--cross svg-icon valign--middle"
height="20"
preserveAspectRatio="xMinYMin meet"
role="presentation"
style={Object {}}
viewBox="0 0 20 20"
width="20"
>
<use
xlinkHref="#icon-cross--small"
/>
</svg>
</Icon>
</span>
</Icon>
</span>
</button>
</Button>
</Button>
<Section
Expand Down
36 changes: 21 additions & 15 deletions src/navigation/Nav.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// eslint-disable no-undef
import PropTypes from 'prop-types';
import * as React from 'react';
import cx from 'classnames';
Expand Down Expand Up @@ -240,7 +241,7 @@ export class Nav extends React.Component {
isNewNavsOrder={
isNewNavsOrder && (localeCode ? localeCode.includes('en') : false)
}
hasNewConnections={connections.hasNewConnections}
hasNewConnections={connections && connections.hasNewConnections}
/>
) : (
<DropdownLoader label={dropdownLoaderLabel} />
Expand Down Expand Up @@ -282,7 +283,11 @@ export class Nav extends React.Component {
};

const getConnectionsIcon = () => (
<img className="proIcon" alt={connections.label} src={CONNECTIONS_ICON} />
<img
className="proIcon"
alt={connections && connections.label}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible that connections can be null or undefined? If so let's have a fallback string value here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep it's possible cause on pro-web connections not implemented and I just try to fix issue for update a lib version, it's not a part of my work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it should be removed when connections will be implemented on pro-web, but idk who deals with this

Copy link
Contributor

@meetup-anton-hulak meetup-anton-hulak Jan 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I see, it will fix the type error, but still will leave alt attribute blank. Why not to use ternary here? connections ? connections.label : 'pro-icon (or some other fallback)

src={CONNECTIONS_ICON}
/>
);

const getNotificationsIcon = () => {
Expand Down Expand Up @@ -399,19 +404,20 @@ export class Nav extends React.Component {
linkClassName: 'navItemLink-pro',
isTargetBlank: true,
},
media.isAtMediumUp && {
shrink: true,
linkTo: connections.link,
label: connections.label,
labelClassName: 'navItem-label-pro',
className: cx('navItem--connections', CLASS_AUTH_ITEM),
linkClassName: 'navItemLink-pro',
counterBadgeClassName: 'navItem--counterBadgeConnections',
icon: getConnectionsIcon(),
hasUpdates: connections.hasNewConnections > 0,
updatesLabel: updatesLabel,
onLinkClick: messages.onLinkClick,
},
media.isAtMediumUp &&
!!connections && {
shrink: true,
linkTo: connections.link,
label: connections.label,
labelClassName: 'navItem-label-pro',
className: cx('navItem--connections', CLASS_AUTH_ITEM),
linkClassName: 'navItemLink-pro',
counterBadgeClassName: 'navItem--counterBadgeConnections',
icon: getConnectionsIcon(),
hasUpdates: connections.hasNewConnections > 0,
updatesLabel: updatesLabel,
onLinkClick: messages.onLinkClick,
},
{
shrink: true,
linkTo: messages.link,
Expand Down
Loading