Skip to content
This repository has been archived by the owner on Nov 4, 2024. It is now read-only.

Feature/cor 1159 collapsible thermometer description #4468

Merged
merged 10 commits into from
Oct 24, 2022
Merged

Feature/cor 1159 collapsible thermometer description #4468

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
cd3cb52
imported CollapsibleSection and imported IndicatorLevelDescription co…
Oct 19, 2022
a9481de
Sanity keys added for this ticket
Jorrik-Klijnsma-Work Oct 21, 2022
e9a58cc
added component structure and elements
Jorrik-Klijnsma-Work Oct 21, 2022
8036ea6
Responsive and sizes
Jorrik-Klijnsma-Work Oct 24, 2022
3f2eb5b
Adjusted to the right colors
Jorrik-Klijnsma-Work Oct 24, 2022
b747deb
Update packages/app/src/components/collapsible/collapsible-section.tsx
Jorrik-Klijnsma-Work Oct 24, 2022
facbd74
Update packages/app/src/domain/topical/components/indicator-level-des…
Jorrik-Klijnsma-Work Oct 24, 2022
c9b303c
fix: resolve review feedback
Jorrik-Klijnsma-Work Oct 24, 2022
703e30e
fix: solved PR feedback
Jorrik-Klijnsma-Work Oct 24, 2022
10a8982
chore: fix single character variable
Jorrik-Klijnsma-Work Oct 24, 2022
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
44 changes: 17 additions & 27 deletions packages/app/src/components/collapsible/collapsible-section.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,20 +6,18 @@ import { isAtBottomOfPage } from '~/utils/is-at-bottom-of-page';
import { isElementAtTopOfViewport } from '~/utils/is-element-at-top-of-viewport';
import { useCollapsible } from '~/utils/use-collapsible';
import { Anchor } from '../typography';
import { colors } from '@corona-dashboard/common';

interface CollapsibleSectionProps extends BoxProps {
summary: string;
children: ReactNode;
id?: string;
hideBorder?: boolean;
textColor?: string;
borderColor?: string;
}

export const CollapsibleSection = ({
summary,
children,
id,
hideBorder,
}: CollapsibleSectionProps) => {
export const CollapsibleSection = ({ summary, children, id, hideBorder, textColor = colors.blue8, borderColor = colors.gray2 }: CollapsibleSectionProps) => {
const section = useRef<HTMLElement>(null);

const collapsible = useCollapsible();
Expand Down Expand Up @@ -53,24 +51,13 @@ export const CollapsibleSection = ({
}, [toggle, id]);

return (
<Box
as="section"
borderTop={hideBorder ? undefined : '1px solid'}
borderTopColor={hideBorder ? undefined : 'gray2'}
id={id}
ref={section}
>
<Box as="section" borderTop={hideBorder ? undefined : '1px solid'} borderTopColor={hideBorder ? undefined : borderColor} id={id} ref={section}>
{collapsible.button(
<Summary>
<Summary textColor={textColor}>
<Box width="100%">
{summary}
{id && (
<StyledAnchor
aria-hidden="true"
tabIndex={-1}
onClick={(e) => e.stopPropagation()}
href={`#${id}`}
>
<StyledAnchor aria-hidden="true" tabIndex={-1} onClick={(e) => e.stopPropagation()} href={`#${id}`}>
Jorrik-Klijnsma-Work marked this conversation as resolved.
Show resolved Hide resolved
#
</StyledAnchor>
)}
Expand All @@ -86,31 +73,34 @@ export const CollapsibleSection = ({

const StyledAnchor = styled(Anchor)(
css({
color: 'gray2',
color: colors.gray2,
px: 3,
py: 1,
width: 0,
textDecoration: 'none',
position: 'absolute',
right: '100%',
'&:hover, &:focus': {
color: 'blue',
color: colors.blue1,
},
})
);

const Summary = styled.button(
interface SummaryProps {
textColor: string;
}
const Summary = styled.button((summaryProps: SummaryProps) =>
css({
display: 'flex',
alignItems: 'flex-start',
justifyContent: 'space-between',
overflow: 'visible',
width: '100%',
m: 0,
p: 3,
margin: 0,
padding: 3,
bg: 'transparent',
border: 'none',
color: 'blue8',
color: summaryProps.textColor,
Jorrik-Klijnsma-Work marked this conversation as resolved.
Show resolved Hide resolved
fontFamily: 'body',
fontWeight: 'bold',
fontSize: '1.25rem',
Expand All @@ -121,7 +111,7 @@ const Summary = styled.button(
'&:focus': {
outlineWidth: '1px',
outlineStyle: 'dashed',
outlineColor: 'blue8',
outlineColor: colors.blue8,
},

[StyledAnchor]: { opacity: 0 },
Expand Down
18 changes: 4 additions & 14 deletions packages/app/src/components/severity-indicator-tile/components/severity-indicator-label.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,29 +12,19 @@ interface SeverityIndicatorLabelProps {
level: SeverityLevel;
}

export const SeverityIndicatorLabel = ({
label,
level,
}: SeverityIndicatorLabelProps) => {
export const SeverityIndicatorLabel = ({ label, level }: SeverityIndicatorLabelProps) => {
return (
<Box
alignItems="center"
display="flex"
justifyContent="flex-start"
my={space[3]}
Jorrik-Klijnsma-Work marked this conversation as resolved.
Show resolved Hide resolved
css={css({ gap: `0 ${space[2]}` })}
>
<Box alignItems="center" display="flex" justifyContent="flex-start" my={space[3]} css={css({ gap: `0 ${space[2]}` })}>
<SeverityIndicatorLevel level={level}>{level}</SeverityIndicatorLevel>

<Text variant="h3">{label}</Text>
</Box>
);
};

const SeverityIndicatorLevel = styled.div`
export const SeverityIndicatorLevel = styled.div`
align-items: center;
background-color: ${({ level }: { level: SeverityLevel }) =>
getSeverityColor(level as SeverityLevels)};
background-color: ${({ level }: { level: SeverityLevel }) => getSeverityColor(level as SeverityLevels)};
border-radius: 50%;
color: ${colors.white};
display: flex;
Expand Down
37 changes: 37 additions & 0 deletions packages/app/src/domain/topical/components/indicator-level-description.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import css from '@styled-system/css';
import { SeverityIndicatorLevel } from '~/components/severity-indicator-tile/components/severity-indicator-label';
import { SeverityLevel } from '~/components/severity-indicator-tile/types';
import { replaceVariablesInText } from '~/utils';
import { asResponsiveArray } from '~/style/utils';
import { Text, BoldText } from '~/components/typography';
import { space } from '~/style/theme';
import { Box } from '~/components/base';

interface IndicatorLevelDescriptionProps {
level: SeverityLevel;
label: string;
description: string;
}

export const IndicatorLevelDescription = ({ level, label, description }: IndicatorLevelDescriptionProps) => {
return (
<li value={level}>
<Box
display="grid"
gridTemplateRows="auto"
gridTemplateColumns={`${space[4]} auto`}
alignItems="center"
marginBottom={4}
css={css({ columnGap: 3, rowGap: asResponsiveArray({ _: 3, sm: 1 }) })}
>
<SeverityIndicatorLevel level={level}>{level}</SeverityIndicatorLevel>
<BoldText fontWeight="bold">{label}</BoldText>
Jorrik-Klijnsma-Work marked this conversation as resolved.
Show resolved Hide resolved
<Text css={css({ gridColumnStart: asResponsiveArray({ _: 1, sm: 2 }), gridColumnEnd: 3 })}>
{replaceVariablesInText(description.split('**').join(''), {
label: label.toLowerCase(),
})}
</Text>
</Box>
</li>
);
};
4 changes: 3 additions & 1 deletion packages/app/src/domain/topical/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,6 @@ export { TopicalMeasureTile } from './components/topical-measure-tile';
export { TopicalSectionHeader } from './components/topical-section-header';
export { TopicalThemeHeader } from './components/topical-theme-header';
export { TopicalTile } from './components/topical-tile';
export * from './components/search';
export { IndicatorLevelDescription } from './components/indicator-level-description';

export * from './components/search';
42 changes: 39 additions & 3 deletions packages/app/src/pages/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,19 @@
import { Box, Spacer } from '~/components/base';
import { MaxWidth } from '~/components';
import styled from 'styled-components';
import { css } from '@styled-system/css';
import { Markdown, MaxWidth } from '~/components';
import { Layout } from '~/domain/layout';
import { Search, TopicalArticlesList, TopicalHeader, TopicalLinksList, TopicalMeasureTile, TopicalSectionHeader, TopicalThemeHeader, TopicalTile } from '~/domain/topical';
import {
Search,
TopicalArticlesList,
TopicalHeader,
TopicalLinksList,
TopicalMeasureTile,
TopicalSectionHeader,
TopicalThemeHeader,
TopicalTile,
IndicatorLevelDescription,
} from '~/domain/topical';
import { isPresent } from 'ts-is-present';
import { Languages, SiteText } from '~/locale';
import { createGetStaticProps, StaticProps } from '~/static-props/create-get-static-props';
Expand All @@ -11,10 +23,11 @@ import { useDynamicLokalizeTexts } from '~/utils/cms/use-dynamic-lokalize-texts'
import { colors } from '@corona-dashboard/common';
import { SeverityIndicatorTile } from '~/components/severity-indicator-tile/severity-indicator-tile';
import { replaceVariablesInText } from '~/utils';
import { SeverityLevels } from '~/components/severity-indicator-tile/types';
import { SeverityLevel, SeverityLevels } from '~/components/severity-indicator-tile/types';
import { useBreakpoints } from '~/utils/use-breakpoints';
import { THERMOMETER_ICON_NAME, TOPICAL_SEVERITY_INDICATOR_TILE_MAX_WIDTH, SEVERITY_LEVELS_LIST } from '~/components/severity-indicator-tile/constants';
import { TrendIcon } from '~/domain/topical/types';
import { CollapsibleSection } from '~/components/collapsible';

const selectLokalizeTexts = (siteText: SiteText) => ({
hospitalText: siteText.pages.hospital_page.nl,
Expand Down Expand Up @@ -86,6 +99,21 @@ const Home = (props: StaticProps<typeof getStaticProps>) => {
levelDescription={textNl.thermometer.indicator.level_description}
trendIcon={textNl.thermometer.indicator.trend_icon as TrendIcon}
/>
<Box my={{ _: 3, md: 4 }} borderBottom={'1px solid'} borderBottomColor={colors.gray3}>
<CollapsibleSection summary={textNl.thermometer.collapsible_title} textColor={colors.black} borderColor={colors.gray3}>
<Box my={3}>
Copy link
Contributor

Choose a reason for hiding this comment

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

How about using a more semantic ol element here (in which each IndicatorLevelDescription component is represented by an li element with a value attrobite) instead of a generic div element (the default element used for a Box component)? The dropdown represents a list of items (severity levels) that are intentionally ordered (their order has impact on how severities are interpreted).

Copy link
Contributor

Choose a reason for hiding this comment

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

I am a bit on the fence about this. The order we're showing these severity level descriptions in, is a human decision. The decision could also be(come): active level first, then the rest (in reverse order, why not?).
The levels could also become different. Now we have 3 which is worse than 2 but not as bad as 4. But what if they decide to get another level that's just as bad as 3, but in a different way?

I'd say safe ordered lists for natural orders such as sequential instructions, where the order is emphasised and use a ul instead. But that's just my opinion. I'll leave it to the developer.

Do make sure to test this properly on different browsers and systems though, because browsers add a number/bullet to lis natively, so making sure they don't show up in none of them can be tricky.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The first part of your question shows me it's the right thing to have it in an ordered list. Because as stated by you and previously the list has an order. And it's not random. It's the same every time, based on a human decision and executed manually. And not something unordered. And the part about sequential instructions? Who decides the order? It's a human decision, getting really abstract really fast.

The second part of your question: I agree testing is an important part of this list behaviour.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am a bit on the fence about this. The order we're showing these severity level descriptions in, is a human decision. The decision could also be(come): active level first, then the rest (in reverse order, why not?). The levels could also become different. Now we have 3 which is worse than 2 but not as bad as 4. But what if they decide to get another level that's just as bad as 3, but in a different way?

I'd say safe ordered lists for natural orders such as sequential instructions, where the order is emphasised and use a ul instead. But that's just my opinion. I'll leave it to the developer.

I get where you are coming from, but this is currently not supported by the implementation of the severity indicator. As of right now, it assumes there is a sequential order of severities based on what types are defined in code. If we would like to prepare for these potential changes like completely shuffling the order of severities (and possibly having the ability to do that on the fly) or having multiple severities of the same level but different, I agree that it would make sense to go with an unordered list as the order of severities could seem completely off (especially if an active severity is are sorted at the top, for example).

I mainly suggested using a semantic element over a div because we recently discussed to apply more semantic elements where possible.

Copy link
Contributor

@VWSCoronaDashboard25 VWSCoronaDashboard25 Oct 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@VWSCoronaDashboard18

The first part of your question shows me it's the right thing to have it in an ordered list. Because as stated by you and previously the list has an order. And it's not random. It's the same every time, based on a human decision and executed manually. And not something unordered. And the part about sequential instructions? Who decides the order? It's a human decision, getting really abstract really fast.

My wording was maybe not on the spot, every website is formed on a human decision, so by continuing that logic in this manner, ul is obsolete and every list on a website should be a ol.

MDN has a nice hint that maybe can help to decide when to use an ol and when an ul

To determine which list to use, try changing the order of the list items; if the meaning changes, use the <ol> element — otherwise you can use <ul>.

In my opinion, changing the order doesn't change the meaning, because we have our own indicator inside that represents the severity level.

Copy link
Contributor

Choose a reason for hiding this comment

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

@VWSCoronaDashboard26

I mainly suggested using a semantic element over a div because we recently discussed to apply more semantic elements where possible.

This, I fully support! I doubt though if we have forced our hand by the way it is coded, but it's fine either way. I just don't fully agree 😜

<OrderedList>
{Object.values(SeverityLevels).map((severityLevel, index) => {
const indicatorTexts = textNl.thermometer[`indicator_for_level_${severityLevel}`];
return (
<IndicatorLevelDescription key={index} level={severityLevel as SeverityLevel} label={indicatorTexts.label} description={indicatorTexts.description} />
);
})}
</OrderedList>
<Markdown content={textNl.thermometer.article_reference} />
</Box>
</CollapsibleSection>
</Box>
</Box>
)}
<Box spacing={{ _: 5, md: 6 }} px={{ _: 3, sm: 4 }}>
Expand Down Expand Up @@ -172,4 +200,12 @@ const Home = (props: StaticProps<typeof getStaticProps>) => {
);
};

const OrderedList = styled.ol(
css({
listStyleType: 'none',
margin: 0,
padding: 0,
})
);

export default Home;
16 changes: 16 additions & 0 deletions packages/cms/src/lokalize/key-mutations.csv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,26 @@ timestamp,action,key,document_id,move_to
2022-10-13T12:16:30.506Z,add,pages.vaccinations_page.vr.vaccination_coverage.top_level_information_block.dates,Hbf18OLBqHEYSTRqYPuVtR,__
2022-10-13T12:16:31.512Z,add,pages.vaccinations_page.vr.vaccination_coverage.top_level_information_block.reference.href,Hbf18OLBqHEYSTRqYPuWBB,__
2022-10-13T12:16:32.533Z,add,pages.vaccinations_page.vr.vaccination_coverage.top_level_information_block.title,hisQZuyoPo60cR6z1YNuqw,__
2022-10-21T08:27:05.448Z,add,pages.topical_page.nl.thermometer.current_indicator_level,K48I3DmxlzVCAHe3YoBCIQ,__
2022-10-21T08:27:06.474Z,add,pages.topical_page.nl.thermometer.indicator_for_level_1.title,i1eZmWfHjT5bLGKFlhIEjW,__
Jorrik-Klijnsma-Work marked this conversation as resolved.
Show resolved Hide resolved
2022-10-21T08:27:07.488Z,add,pages.topical_page.nl.thermometer.indicator_for_level_1.description,NYpb1u6P0tqViLcixo7gEh,__
2022-10-21T08:27:08.466Z,add,pages.topical_page.nl.thermometer.indicator_for_level_1.label,NYpb1u6P0tqViLcixo7gxG,__
2022-10-21T08:27:09.489Z,add,pages.topical_page.nl.thermometer.indicator_for_level_2.title,i1eZmWfHjT5bLGKFlhIFdW,__
2022-10-21T08:27:10.505Z,add,pages.topical_page.nl.thermometer.indicator_for_level_2.description,K48I3DmxlzVCAHe3YoBCYs,__
2022-10-21T08:27:11.516Z,add,pages.topical_page.nl.thermometer.indicator_for_level_2.label,K48I3DmxlzVCAHe3YoBCma,__
2022-10-21T08:27:12.533Z,add,pages.topical_page.nl.thermometer.indicator_for_level_3.title,K48I3DmxlzVCAHe3YoBDBG,__
2022-10-21T08:27:13.553Z,add,pages.topical_page.nl.thermometer.indicator_for_level_3.description,K48I3DmxlzVCAHe3YoBDOy,__
2022-10-21T08:27:14.559Z,add,pages.topical_page.nl.thermometer.indicator_for_level_3.label,K48I3DmxlzVCAHe3YoBDXC,__
2022-10-21T08:27:15.571Z,add,pages.topical_page.nl.thermometer.indicator_for_level_4.title,i1eZmWfHjT5bLGKFlhIGQW,__
2022-10-21T08:27:16.560Z,add,pages.topical_page.nl.thermometer.indicator_for_level_4.description,i1eZmWfHjT5bLGKFlhIGb1,__
2022-10-21T08:27:17.602Z,add,pages.topical_page.nl.thermometer.indicator_for_level_4.label,i1eZmWfHjT5bLGKFlhIGlW,__
2022-10-21T08:27:18.593Z,add,pages.topical_page.nl.thermometer.article_reference,NYpb1u6P0tqViLcixo7iYI,__
2022-10-21T08:27:19.629Z,add,pages.topical_page.nl.thermometer.collapsible_title,K48I3DmxlzVCAHe3YoBE46,__
2022-10-20T08:52:25.474Z,delete,pages.intensive_care_page.nl.kpi_bedbezetting.barscale_screenreader_text,jDbv30Y7XYH5kl8ZaovZD6,__
2022-10-19T11:41:12.347Z,add,pages.topical_page.nl.thermometer.indicator.source_label,65tvoiVcF0tDaAwdlNm7IM,__
2022-10-19T11:41:13.292Z,add,pages.topical_page.nl.thermometer.indicator.dates_label,Z4981ecxxAauoAYZ8R2IO1,__
2022-10-19T11:41:14.265Z,add,pages.topical_page.nl.thermometer.indicator.trend_icon.direction,Z4981ecxxAauoAYZ8R2IYZ,__
2022-10-19T11:41:15.281Z,add,pages.topical_page.nl.thermometer.indicator.trend_icon.color,Z4981ecxxAauoAYZ8R2J0h,__
2022-10-19T11:41:16.315Z,add,pages.topical_page.nl.thermometer.indicator.level_description,65tvoiVcF0tDaAwdlNm7cu,__
2022-10-19T11:41:16.318Z,delete,pages.topical_page.nl.thermometer.indicator.footerText,yWf5NF9ZnuFvbcgji0hw3P,__