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

Conversation

Jorrik-Klijnsma-Work
Copy link
Contributor

Created the thermometer collapsible.
This is a new element and looks like this:

COLLAPSED
Screenshot 2022-10-24 at 09 59 19

EXPANDED
Screenshot 2022-10-24 at 09 59 15

@@ -82,6 +93,17 @@ const Home = (props: StaticProps<typeof getStaticProps>) => {
label={textNl.thermometer.indicator.label}
footerText={textNl.thermometer.indicator.footerText}
/>
<Box my={{ _: 3, md: 4 }} borderBottom={'1px solid'} borderBottomColor={colors.gray3}>
<CollapsibleSection summary={textNl.thermometer.collapsible_title} textColorOverride={colors.black} borderColorOverride={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

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 😜

Copy link
Contributor

@VWSCoronaDashboard26 VWSCoronaDashboard26 left a comment

Choose a reason for hiding this comment

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

On small comment on a remnant of a previous approach. Please have a look at the last comments.

Copy link
Contributor

@APW26 APW26 left a comment

Choose a reason for hiding this comment

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

Approved with one small non-blocking comment

@Jorrik-Klijnsma-Work Jorrik-Klijnsma-Work merged commit 7fde2dc into feature/cor-xxx-coronathermometer-changes Oct 24, 2022
@Jorrik-Klijnsma-Work Jorrik-Klijnsma-Work deleted the feature/COR-1159-collapsible-thermometer-description branch October 24, 2022 14:20
Jorrik-Klijnsma-Work added a commit that referenced this pull request Oct 26, 2022
* COR-1135: dynamic-thermometer (#4464)

* feat: new descriptions added to thermometer

* feat: fixed color mapping and changes some logic

* feat: add icon direction function and type

Co-authored-by: VWSCoronaDashboard27 <[email protected]>
Co-authored-by: VWSCoronaDashboard19 <[email protected]>

* Feature/cor 1159 collapsible thermometer description (#4468)

* imported CollapsibleSection and imported IndicatorLevelDescription component

* Sanity keys added for this ticket

* added component structure and elements

* Responsive and sizes

* Adjusted to the right colors

* Update packages/app/src/components/collapsible/collapsible-section.tsx

Co-authored-by: VWSCoronaDashboard25 <[email protected]>

* Update packages/app/src/domain/topical/components/indicator-level-description.tsx

Co-authored-by: VWSCoronaDashboard25 <[email protected]>

* fix: resolve review feedback

* fix: solved PR feedback

* chore: fix single character variable

Co-authored-by: VWSCoronaDashboard21 <[email protected]>
Co-authored-by: VWSCoronaDashboard25 <[email protected]>

* Merging sanity changes from both branches

* sanity changes

* Fix: fixing stuck focus-state

* FIX: Fixing breakpoint glitching

* FIX: no user selecting

* FEAT: Move the article link outside the collapsible

* FIX: variable typo

Co-authored-by: VWSCoronaDashboard27 <[email protected]>
Co-authored-by: VWSCoronaDashboard27 <[email protected]>
Co-authored-by: J <[email protected]>
Co-authored-by: VWSCoronaDashboard21 <[email protected]>
Co-authored-by: VWSCoronaDashboard25 <[email protected]>
VWSCoronaDashboard26 pushed a commit that referenced this pull request Oct 28, 2022
* COR-1135: dynamic-thermometer (#4464)

* feat: new descriptions added to thermometer

* feat: fixed color mapping and changes some logic

* feat: add icon direction function and type

Co-authored-by: VWSCoronaDashboard27 <[email protected]>
Co-authored-by: VWSCoronaDashboard19 <[email protected]>

* Feature/cor 1159 collapsible thermometer description (#4468)

* imported CollapsibleSection and imported IndicatorLevelDescription component

* Sanity keys added for this ticket

* added component structure and elements

* Responsive and sizes

* Adjusted to the right colors

* Update packages/app/src/components/collapsible/collapsible-section.tsx

Co-authored-by: VWSCoronaDashboard25 <[email protected]>

* Update packages/app/src/domain/topical/components/indicator-level-description.tsx

Co-authored-by: VWSCoronaDashboard25 <[email protected]>

* fix: resolve review feedback

* fix: solved PR feedback

* chore: fix single character variable

Co-authored-by: VWSCoronaDashboard21 <[email protected]>
Co-authored-by: VWSCoronaDashboard25 <[email protected]>

* Merging sanity changes from both branches

* sanity changes

* Fix: fixing stuck focus-state

* FIX: Fixing breakpoint glitching

* FIX: no user selecting

* FEAT: Move the article link outside the collapsible

* FIX: variable typo

Co-authored-by: VWSCoronaDashboard27 <[email protected]>
Co-authored-by: VWSCoronaDashboard27 <[email protected]>
Co-authored-by: J <[email protected]>
Co-authored-by: VWSCoronaDashboard21 <[email protected]>
Co-authored-by: VWSCoronaDashboard25 <[email protected]>
hasan-ozaynaci added a commit that referenced this pull request Nov 2, 2022
* Coronathermometer component extension with collapsible (#4470)

* COR-1135: dynamic-thermometer (#4464)

* feat: new descriptions added to thermometer

* feat: fixed color mapping and changes some logic

* feat: add icon direction function and type

Co-authored-by: VWSCoronaDashboard27 <[email protected]>
Co-authored-by: VWSCoronaDashboard19 <[email protected]>

* Feature/cor 1159 collapsible thermometer description (#4468)

* imported CollapsibleSection and imported IndicatorLevelDescription component

* Sanity keys added for this ticket

* added component structure and elements

* Responsive and sizes

* Adjusted to the right colors

* Update packages/app/src/components/collapsible/collapsible-section.tsx

Co-authored-by: VWSCoronaDashboard25 <[email protected]>

* Update packages/app/src/domain/topical/components/indicator-level-description.tsx

Co-authored-by: VWSCoronaDashboard25 <[email protected]>

* fix: resolve review feedback

* fix: solved PR feedback

* chore: fix single character variable

Co-authored-by: VWSCoronaDashboard21 <[email protected]>
Co-authored-by: VWSCoronaDashboard25 <[email protected]>

* Merging sanity changes from both branches

* sanity changes

* Fix: fixing stuck focus-state

* FIX: Fixing breakpoint glitching

* FIX: no user selecting

* FEAT: Move the article link outside the collapsible

* FIX: variable typo

Co-authored-by: VWSCoronaDashboard27 <[email protected]>
Co-authored-by: VWSCoronaDashboard27 <[email protected]>
Co-authored-by: J <[email protected]>
Co-authored-by: VWSCoronaDashboard21 <[email protected]>
Co-authored-by: VWSCoronaDashboard25 <[email protected]>

* WIP;

* WIP;

* WIP;

* WIP;

* feat(severity-indicator): close to finalising the component;

* feat(severity-indicator): added hover functionality to timeline events;

* feat(severity-indicator): updated Sanity keys;

* feat(severity-indicator): cleanup;

* feat(severity-indicator): reverted auto-generated change;

* feat(severity-indicator): PR feedback;

Co-authored-by: HO <[email protected]>
Co-authored-by: VWSCoronaDashboard27 <[email protected]>
Co-authored-by: VWSCoronaDashboard27 <[email protected]>
Co-authored-by: J <[email protected]>
Co-authored-by: VWSCoronaDashboard21 <[email protected]>
Co-authored-by: VWSCoronaDashboard25 <[email protected]>
Co-authored-by: VWSCoronaDashboard26 <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants