feat(AdvancedDataCard): support for href/to #859

brunocezar-tlf · 2023-08-22T18:18:55Z

PR for Advanced Data Card accessibility testing

github-actions · 2023-08-22T18:21:31Z

Size stats

	master	this branch	diff
Total JS	9.58 MB	9.58 MB	+1.21 kB
JS without icons	1.02 MB	1.02 MB	+1.21 kB
Lib overhead	126 kB	126 kB	0 B
Lib overhead (gzip)	32.1 kB	32.1 kB	0 B

github-actions · 2023-08-22T18:23:05Z

Deploy preview for mistica-web ready!

✅ Preview
https://mistica-qjp7nfg9b-tuentisre.vercel.app

Built with commit 08777c2.
This pull request is being automatically deployed with vercel-action

github-actions · 2023-08-22T18:24:43Z

Accessibility report
✔️ No issues found

ℹ️ You can run this locally by executing yarn audit-accessibility.

github-actions · 2023-08-22T18:31:41Z

Screenshot tests report

✔️ All passing

atabel · 2023-08-24T08:47:44Z

src/community/advanced-data-card.tsx

@@ -289,6 +281,7 @@ export const AdvancedDataCard = React.forwardRef<HTMLDivElement, AdvancedDataCar
                        maybe
                        className={classNames(styles.touchableContainer, {[styles.hoverEffect]: !!onPress})}
                        aria-label={ariaLabel}
+                        role="link"


why role link?
Touchable renders as html <button> (role button) when used with onPress prop and as html <a> (role link) when used with href/to prop . If you want a link, I think the best approach is adding href/to support to AdvancedDataCard as we have in other cards

Hello Abel,

This PR is still under review of accessibility team, we upload here to generate the preview link for them.
Most grateful for your contribution at this time :)

the role='link' was to be able to adjust the accessibility semantics without changing any properties, but I implemented the href according to your suggestion

yceballost · 2023-10-06T14:27:17Z

playroom/snippets.tsx

@@ -2343,7 +2343,7 @@ const advancedDataCardSnippets = [
          }
          footerText="footer text"
          onClose={() => window.alert("close")}
-          onPress={() => window.alert("click")}
+          href="asdf"


Suggested change

href="asdf"

href="https://google.com"

yceballost · 2023-10-06T14:28:30Z

...tsx-advanced-data-card-without-stacking-group-with-top-actions-and-too-long-title-1-snap.png

this is not expected, is it?

was not expected, but the slightly larger size is due to the touchable being moved into the internal component that encompasses pretitle, title etc.

yceballost · 2023-10-06T14:34:59Z

src/community/__stories__/advanced-data-card-story.tsx

just curious, my focus of the review is not this kind of technical details but I would like to understand why the replacement of onPress by href and does not support both properties (like the rest of the cards)

The reason for changing from onPress to href is due to the screen reader, with onPress it would be read as a button, while href as a link which would be more suitable for accessibility

atabel · 2023-10-11T15:45:39Z

src/community/advanced-data-card.tsx

+                href={href}
+                aria-label={ariaLabel}
+            >
+                <div />


what's this empty div? and why are you creating an empty link (Touchable)?

The empty div is because the touchable must have a child, and the reason the touchable is there is so the touchable does not interfere with the screen reader's reading order.

Touchable is a link, as it must be read as such in the screen reader, where it receives the link from the href prop previously as onPress.

atabel · 2023-10-11T15:46:21Z

src/community/advanced-data-card.css.ts

@@ -38,9 +38,18 @@ export const actions = style([
    },
 ]);

+export const touchableAccessibility = style({


please use a more semantic name

atabel · 2023-10-11T15:46:35Z

src/community/advanced-data-card.css.ts

@@ -95,6 +103,8 @@ export const cardContentStyle = style([
    },
 ]);

+export const zindex = style({zIndex: -20});


The negative z-index is so that images in slots (like SimpleBlock) don't overlap the Touchable

this number doesn't make sense to me. I'm sure you can reorder the elements, use different stacking contexts or using a more sensible zIndex value than -20

atabel · 2023-10-11T15:47:59Z

src/community/advanced-data-card.tsx

-    onPress?: () => void;
+    href?: string | undefined;


this is a breaking change, we should avoid breaking changes, otherwise we'd need to generate a major version. Also why don't you support href/onPress/to as we do with other cards?

The change from onPress to href is so that on the screen reader it can be read as a link instead of a button.

Hi @atabel,
Is something pending on our side?

We can't make a major version just for this breaking change, we need to support onPress + href, and we also can include to to have parity with the rest of the cards. But we need to keep onPress because in master is already existing and there are teams using this prop.

@EduardoLafuente @BrunoGuimaraes103

atabel · 2023-11-08T13:14:46Z

src/community/advanced-data-card.css.ts

@@ -95,6 +103,8 @@ export const cardContentStyle = style([
    },
 ]);

+export const zindex = style({zIndex: -20});


this number doesn't make sense to me. I'm sure you can reorder the elements, use different stacking contexts or using a more sensible zIndex value than -20

atabel · 2023-11-08T13:15:57Z

src/community/advanced-data-card.tsx

@@ -261,7 +290,7 @@ export const AdvancedDataCard = React.forwardRef<HTMLDivElement, AdvancedDataCar
        },
        ref
    ) => {
-        const isTouchable = !!onPress;
+        const isTouchable = !!href || !!onPress;


what about to ?

atabel · 2023-11-08T13:17:35Z

src/community/advanced-data-card.tsx

+                    tabIndex={0}
+                    maybe
+                    className={classNames(styles.touchableArea)}
+                    as="a"


not needed, Touchable renders an <a> when used with href or to

atabel · 2023-11-08T13:21:02Z

src/community/advanced-data-card.tsx

    onPress?: () => void;
+    href?: string | undefined;
+    to?: undefined;


to/href/onPress shouldn't be allowed at the same time. You can use ExclusifyUnion to help you with this.
Please, review how we do the same in other mistica cards: https://github.com/Telefonica/mistica-web/blob/master/src/card.tsx#L416-L425

atabel · 2023-11-15T09:29:05Z

src/community/advanced-data-card.tsx

    onPress?: () => void;
+    href?: string | undefined;
+    to?: undefined;


remove to/href/onPress from common props, you are already defining them in the ExclusifyUnion

atabel · 2023-11-15T09:30:34Z

src/community/advanced-data-card.tsx

@@ -43,9 +46,13 @@ type CardContentProps = {
    subtitleLinesMax?: number;
    description?: string;
    descriptionLinesMax?: number;
+    href?: string;
+    to?: undefined;


to shouldn't be undefined here.
You should use ExclusifyUnion here too, like you do in AdvancedDataCardProps

atabel · 2023-11-15T09:39:46Z

src/community/advanced-data-card.tsx

+            {href ? (
+                <Touchable
+                    tabIndex={0}
+                    maybe
+                    className={classNames(styles.touchableArea)}
+                    href={href}
+                    to={to}
+                    aria-label={ariaLabel}
+                >
+                    <div />
+                </Touchable>
+            ) : (
+                <Touchable
+                    tabIndex={0}
+                    maybe
+                    className={classNames(styles.touchableArea)}
+                    onPress={onPress}
+                    to={to}
+                    aria-label={ariaLabel}
                >
-                    {title}
-                </Text>
+                    <div />
+                </Touchable>
+            )}


you can simplify this by doing something like this: https://github.com/Telefonica/mistica-web/blob/master/src/card.tsx#L511

# [14.32.0](v14.31.1...v14.32.0) (2023-11-16) ### Bug Fixes * **ScreenReaderOnly:** Fix ScreenReaderOnly positioning to avoid scrolling issues ([#942](#942)) ([f6537e9](f6537e9)) ### Features * **AdvancedDataCard:** support for href/to ([#859](#859)) ([ba5eafb](ba5eafb))

tuentisre · 2023-11-16T16:27:05Z

🎉 This PR is included in version 14.32.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

brunocezar-tlf added 18 commits June 6, 2023 14:24

create slider component

c2b0328

Merge remote-tracking branch 'origin' into slider-component

32d9eaa

correction merger

7d74924

Merge branch 'master' of https://github.com/Telefonica/mistica-web

69dd26a

Merge branch 'master' of https://github.com/Telefonica/mistica-web

b605333

adjustment for merger with chip

8d690bf

re-test theme variant

fd5c14e

Merge branch 'master' of https://github.com/Telefonica/mistica-web

a4abeff

Merge branch 'master' of https://github.com/Telefonica/mistica-web

9c3aeb4

Merge branch 'master' of https://github.com/Telefonica/mistica-web

15dbcf0

Merge branch 'master' of https://github.com/Telefonica/mistica-web

8d9a81d

Merge branch 'master' of https://github.com/Telefonica/mistica-web

7f5d8c7

Merge branch 'master' of https://github.com/Telefonica/mistica-web

6e13346

Merge branch 'master' of https://github.com/Telefonica/mistica-web

4d82c95

Merge branch 'master' of https://github.com/Telefonica/mistica-web

2ea8b2c

Merge branch 'master' of https://github.com/Telefonica/mistica-web

9e92ad0

Merge branch 'master' of https://github.com/Telefonica/mistica-web

aca4448

fix: accessibility issues

546e400

atabel reviewed Aug 24, 2023

View reviewed changes

brunocezar-tlf added 7 commits August 24, 2023 10:45

fix: switch from onpress to href

bc36948

fix: onClose and test arialabel

18760a4

fix: test-screenshot

3049e83

re adjustment touchable

dddc962

Merge branch 'master' into accessibility-advanced-data-card

c77c27d

resolve conflits

600ea53

fix height touchable

bd71459

brunocezar-tlf requested a review from yceballost October 5, 2023 17:40

EduardoLafuente marked this pull request as ready for review October 6, 2023 13:39

brunocezar-tlf changed the title ~~Accessibility Advanced Data Card~~ feat(DataCard Advanced): fix Accesibility Oct 6, 2023

yceballost requested changes Oct 6, 2023

View reviewed changes

fix snippet

55b4b5a

brunocezar-tlf requested a review from yceballost October 6, 2023 16:34

brunocezar-tlf changed the title ~~feat(DataCard Advanced): fix Accesibility~~ Fix(DataCard Advanced): Accessibility Oct 6, 2023

yceballost approved these changes Oct 10, 2023

View reviewed changes

atabel reviewed Oct 11, 2023

View reviewed changes

fix: semantic name

a0a8239

brunocezar-tlf requested a review from atabel October 16, 2023 20:24

add onpress and to

b8cb532

atabel requested changes Nov 8, 2023

View reviewed changes

brunocezar-tlf added 3 commits November 8, 2023 13:05

fix: exclusifyunion and value zindex

0a7aeab

adjust exclusifyunion

998ed0d

fix prettier

b294c2e

brunocezar-tlf requested a review from atabel November 8, 2023 16:35

atabel reviewed Nov 15, 2023

View reviewed changes

atabel changed the title ~~Fix(DataCard Advanced): Accessibility~~ feat(AdvancedDataCard): support for href/to Nov 15, 2023

brunocezar-tlf added 2 commits November 16, 2023 09:53

fix touchableprops

5c34159

fix arialabel

08777c2

brunocezar-tlf requested a review from atabel November 16, 2023 13:30

atabel approved these changes Nov 16, 2023

View reviewed changes

atabel added this pull request to the merge queue Nov 16, 2023

Merged via the queue into master with commit ba5eafb Nov 16, 2023

atabel deleted the accessibility-advanced-data-card branch November 16, 2023 16:19

tuentisre added the released label Nov 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(AdvancedDataCard): support for href/to #859

feat(AdvancedDataCard): support for href/to #859

brunocezar-tlf commented Aug 22, 2023

github-actions bot commented Aug 22, 2023 •

edited

Loading

github-actions bot commented Aug 22, 2023 •

edited

Loading

github-actions bot commented Aug 22, 2023 •

edited

Loading

github-actions bot commented Aug 22, 2023 •

edited

Loading

atabel Aug 24, 2023

EduardoLafuente Aug 24, 2023

brunocezar-tlf Aug 24, 2023

yceballost Oct 6, 2023

yceballost Oct 6, 2023

brunocezar-tlf Oct 6, 2023

yceballost Oct 6, 2023

brunocezar-tlf Oct 6, 2023

atabel Oct 11, 2023

brunocezar-tlf Oct 11, 2023 •

edited

Loading

atabel Oct 11, 2023

atabel Oct 11, 2023

brunocezar-tlf Oct 11, 2023

atabel Nov 8, 2023

atabel Oct 11, 2023

brunocezar-tlf Oct 11, 2023

EduardoLafuente Oct 25, 2023

yceballost Oct 25, 2023 •

edited

Loading

atabel Nov 8, 2023

atabel Nov 8, 2023

atabel Nov 8, 2023

atabel Nov 8, 2023

atabel Nov 15, 2023

atabel Nov 15, 2023 •

edited

Loading

atabel Nov 15, 2023

tuentisre commented Nov 16, 2023

feat(AdvancedDataCard): support for href/to #859

feat(AdvancedDataCard): support for href/to #859

Conversation

brunocezar-tlf commented Aug 22, 2023

github-actions bot commented Aug 22, 2023 • edited Loading

github-actions bot commented Aug 22, 2023 • edited Loading

github-actions bot commented Aug 22, 2023 • edited Loading

github-actions bot commented Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brunocezar-tlf Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yceballost Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atabel Nov 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tuentisre commented Nov 16, 2023

github-actions bot commented Aug 22, 2023 •

edited

Loading

github-actions bot commented Aug 22, 2023 •

edited

Loading

github-actions bot commented Aug 22, 2023 •

edited

Loading

github-actions bot commented Aug 22, 2023 •

edited

Loading

brunocezar-tlf Oct 11, 2023 •

edited

Loading

yceballost Oct 25, 2023 •

edited

Loading

atabel Nov 15, 2023 •

edited

Loading