Feature/#79 cards case

[joostvanviegen]

This pull request aims to modify the card component to adhere to the designs of the Card Case variant.

[joostvanviegen]

@joostvanviegen I also noticed that the focus border around the arrow only shows up for card/case and not for the card/default. Is this intentional?

Is now fixed

[joostvanviegen]

@joostvanviegen another one, this time a bit of a trickier one though: you added tabindex="0" to make the element focusable by keyboard, and a onClick prop to make it clickable. I have a few issues with this though:

* I feel like in our use-case a card would almost always be a link which we would want to define as an `<a>` tag with an `href` instead of a `onClick` function. Usually you would not want to wrap the whole card in an `<a>`-tag as this creates a bit of a screenreader nightmare though. I would personally prefer placing the title (and maybe subtitle) in an `<a>` tag with an `href` attribute. Then add a JS `focus` and `blur` listener which adds/removes the `denhaag-card--focus` class to the card providing the focus border, and a JS `click` listener to pass through the click to the `a` tag when the card is clicked. Furthermore you should add a `cursor: pointer;` rule to the `.denhaag-card` CSS to actually transform the mouse into a pointer when the card is hovered to indicate it is clickable as by default this only happens when hovering a `<a>` or `<button>`. @Robbert how do you look at this?

* And when deciding to work with the `onClick` function (which again, I don't prefer in this scenario): you have made the card focusable by adding `tabindex`, so at least it is reachable by tabbing through the page, however nothing in the HTML indicates that this element is actually clickable. Sure, tabindex makes it focusable but the fact that there is a `onClick` listener here can not be discerned from the HTML as it is simply a bunch of `<div>` and `<p>` tags. This means screenreaders and other a11y tools will not actually detect the card as something the user can click. This can be fixed by either adding a `<button>` tag (which is preferred) somewhere (preferably again with little content, so for example only around the title and subtitle). If for some reason this would not be possible you can add the `role="button"` attribute to a not by default clickable html tag (so in this case a `div`) which will identify it to screenreaders as something the user can click. Using the `role` attribute for this should however only be considered in scenarios where it is impossible to use the existing HTML tag for whatever reason.

* The card is now focusable when no `onClick` is provided because of the `tabindex="0"`: it feels a bit weird to be able to focus something which doesn't actually do anything. Above i mentioned the `cursor: pointer` CSS rule, but this also should only happen when the card is actually clickable. I feel like this component should always have an action (either `button` with an `onClick` or an `a` with an `href`, see above). Right now you can create a card which does nothing because you don't have to provide a `onClick` prop. I feel like we should make it required (either `onClick` or `href`, depending on what we decide based on the discussion which will follow on my comments above).

@Gemeente-DenHaag/mdh-rysst A quick reminder to keep an eye out for a11y problems with our component implementations!

Haven't gotten around to this one because of all the other comments but will do this next week.

[mjcarsjens]

Making some nice progress with resolving the comments! lmk if you need help with the a11y comment

[joostvanviegen]

@joostvanviegen another one, this time a bit of a trickier one though: you added tabindex="0" to make the element focusable by keyboard, and a onClick prop to make it clickable. I have a few issues with this though:

* I feel like in our use-case a card would almost always be a link which we would want to define as an `<a>` tag with an `href` instead of a `onClick` function. Usually you would not want to wrap the whole card in an `<a>`-tag as this creates a bit of a screenreader nightmare though. I would personally prefer placing the title (and maybe subtitle) in an `<a>` tag with an `href` attribute. Then add a JS `focus` and `blur` listener which adds/removes the `denhaag-card--focus` class to the card providing the focus border, and a JS `click` listener to pass through the click to the `a` tag when the card is clicked. Furthermore you should add a `cursor: pointer;` rule to the `.denhaag-card` CSS to actually transform the mouse into a pointer when the card is hovered to indicate it is clickable as by default this only happens when hovering a `<a>` or `<button>`. @Robbert how do you look at this?

* And when deciding to work with the `onClick` function (which again, I don't prefer in this scenario): you have made the card focusable by adding `tabindex`, so at least it is reachable by tabbing through the page, however nothing in the HTML indicates that this element is actually clickable. Sure, tabindex makes it focusable but the fact that there is a `onClick` listener here can not be discerned from the HTML as it is simply a bunch of `<div>` and `<p>` tags. This means screenreaders and other a11y tools will not actually detect the card as something the user can click. This can be fixed by either adding a `<button>` tag (which is preferred) somewhere (preferably again with little content, so for example only around the title and subtitle). If for some reason this would not be possible you can add the `role="button"` attribute to a not by default clickable html tag (so in this case a `div`) which will identify it to screenreaders as something the user can click. Using the `role` attribute for this should however only be considered in scenarios where it is impossible to use the existing HTML tag for whatever reason.

* The card is now focusable when no `onClick` is provided because of the `tabindex="0"`: it feels a bit weird to be able to focus something which doesn't actually do anything. Above i mentioned the `cursor: pointer` CSS rule, but this also should only happen when the card is actually clickable. I feel like this component should always have an action (either `button` with an `onClick` or an `a` with an `href`, see above). Right now you can create a card which does nothing because you don't have to provide a `onClick` prop. I feel like we should make it required (either `onClick` or `href`, depending on what we decide based on the discussion which will follow on my comments above).

@Gemeente-DenHaag/mdh-rysst A quick reminder to keep an eye out for a11y problems with our component implementations!

I have implemented a fix from the article suggested by @Robbert. @mjcarsjens Would be nice if you could look at it again to see if you agree with this solution.

[mjcarsjens]

2 small comments left but once these are resolved I think we can call this one finished, nice work!

[mjcarsjens]

Nice use of the <a> tag, looks like this is a solid setup wrt a11y!
Two questions:

• What made you decide to place the HTML of the <a>-tag in a variable instead of just right here?
• Might be a bit of a discussion point whether or not a <p> HTML tag is actually semantically correct here. As the name implies <p> is a paragraph tag, used to semantically markup a piece of text as a paragraph. In my eyes a single line of text for a title or a subtitle does not make a paragraph which makes me think this is not actually semantically correct HTML use. Not sure what the best approach would be in this construction though as the Typography component is necessary for the styling. My first thought is to use a extra <div> for the block, with a Typography component which uses a HTML span.

[DanielvanVliet]

For the first point, I think this helps to make clear where the react Ref is used and we should stick to this.
For the second point, in the future we should refactor CardCase to make use of the updated Typography component from our library. We can move the discussion to that refactoring.
#253

Created new issue for comment