Info Panel Container #67
Conversation
There was a problem hiding this comment.
I would be curious to know how people feel about adding snapshot tests. I couldn't think of a cleaner way to test these container components as they are so simple and there's not really any 'functionality' to test.
There was a problem hiding this comment.
I have used them in the past. They can be useful in cases like this. They become a frustration when the snapshots cover a large part of an application fail if anything changes but I don't think that's the case here.
There was a problem hiding this comment.
On top of this, can you think of a way to verify that the different positions work?
There was a problem hiding this comment.
Not really as it's purely achieved via CSS. My thinking is that this sort of thing should be verified with visual regression testing through storybook which would be a nice thing to have but we're not there yet.
There was a problem hiding this comment.
I have used them in the past. They can be useful in cases like this. They become a frustration when the snapshots cover a large part of an application fail if anything changes but I don't think that's the case here.
| import { ReactElement } from "react"; | ||
| import styles from "./InfoPanelContainer.module.css"; | ||
|
|
||
| type InfoPanelProps = { |
There was a problem hiding this comment.
Position prop accepts any string but only a limited set values work. Should use a union type.
Also consider adding aria-label and role accessibility (although I'm not exactly sure what role should default to
type Position = 'topLeft' | 'topRight' | 'centerLeft' | 'centerRight' | 'bottomLeft' | 'bottomRight';
type InfoPanelProps = {
children: ReactElement,
position: Position,
"aria-label": string,
role?: string
}
export const InfoPanelContainer = ({
children,
position = 'centerRight',
"aria-label" = 'Info panel',
role = 'complementary?, tooltip? etc'
}: InfoPanelProps) => (
<div
className={`${styles.infoPanelContainer} ${styles[position]}`}
role={role}
aria-label={ariaLabel}
>
{children}
</div>
);There was a problem hiding this comment.
Personally I don't think we should be applying any aria attributes to this as it is only a 'presentation' container. I don't think any role truly fits the use case of this element other than presentation (which is unnecessary on a div and also should not include an aria label ) The accessibility requirements would be purely down to the contents within the container.
All that being said, best practice here might be to use a Section or Aside tag instead of a div. What do you think?
There was a problem hiding this comment.
Ok, that makes sense. I like the idea of using a Section or Aside tag.
There was a problem hiding this comment.
On top of this, can you think of a way to verify that the different positions work?
|
|
||
| export const InfoPanelContainer = ({children, position='centerRight'}:InfoPanelProps) => ( | ||
| <div className={`${styles.infoPanelContainer} ${styles[position]}`}> | ||
| <div>{children}</div> |
There was a problem hiding this comment.
Is the <div> element necessary here?
| flex-direction: column; | ||
| box-shadow: 0 3px 10px rgb(0 0 0 / 0.2); | ||
| border-radius: 8px; | ||
| z-index: 1; |
There was a problem hiding this comment.
What is z-index relative to? How will this interact with other applications?
I'm not an expert on this but maybe there's more you could specify for the stacking context? isolation would be one example
There was a problem hiding this comment.
The idea for this component was that it would be used to create a semi-permanent card somewhere within the map container. This could be used a a permanent element or as a temporary one to be displayed after an event as it is currently being used in APEx.
Whilst not sticking exactly to the concept from https://mapuipatterns.com/info-panel/ this was the inspiration for this component.
| align-self: center; | ||
| flex-direction: column; | ||
| box-shadow: 0 3px 10px rgb(0 0 0 / 0.2); | ||
| border-radius: 8px; |
There was a problem hiding this comment.
Can we use a border radius from theme.css? I added --border-radius-input in this datepicker PR but it may not be the best name/most flexible approach
There was a problem hiding this comment.
Good shout. I had intended to use this.
…neric string type
Description
Adds an initial version of an info panel container component for use within the map container grid.
Type of change
How Has This Been Tested?
Checklist: