Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fjern massa onødvendig javascript fra Collapse #1872

Merged
merged 4 commits into from
Apr 15, 2024
Merged

Conversation

pethel
Copy link
Contributor

@pethel pethel commented Apr 9, 2024

Dagens Collapse har en svaghet. Accordion har ett proplem som kom up på vår Slack. Det går ikke att att legge til innehold dynasmisk. Jag tror det skyldes att dagens Collpase bergener højden som skall animeras med javascript.

 setHeight(`${content.current.scrollHeight}px`);

Når man legger til nytt innehold så ær det forsatt denne bergenande højden som blir stående.

Vi skall se om dette hjelper. Det er oansett en mye elagantare løsning med mye mindre kode synes jag.
Bytta også fra default til named export for det er vel i stort sett det som er brukt andre steder. Det måtte oansett bli en breaking change pga av ny markup.

Det er vell denne jag bruker.
https://caniuse.com/mdn-css_properties_grid-template-rows_animation

@pethel pethel requested a review from a team as a code owner April 9, 2024 18:54
@pethel pethel force-pushed the develop_fix-collapse branch from 27b096f to 9796dd7 Compare April 9, 2024 18:56
Copy link

github-actions bot commented Apr 9, 2024

Azure Static Web Apps: Your stage site is ready! Visit it here: https://black-beach-0d62d0d03-1872.westeurope.2.azurestaticapps.net

@pethel pethel force-pushed the develop_fix-collapse branch from 9796dd7 to 3930e28 Compare April 9, 2024 18:59
Copy link

github-actions bot commented Apr 9, 2024

Azure Static Web Apps: Your stage site is ready! Visit it here: https://black-beach-0d62d0d03-1872.westeurope.2.azurestaticapps.net

1 similar comment
Copy link

github-actions bot commented Apr 9, 2024

Azure Static Web Apps: Your stage site is ready! Visit it here: https://black-beach-0d62d0d03-1872.westeurope.2.azurestaticapps.net

@pethel pethel force-pushed the develop_fix-collapse branch from 3930e28 to 0f54d79 Compare April 10, 2024 07:33
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://black-beach-0d62d0d03-1872.westeurope.2.azurestaticapps.net

@HeleneKassandra
Copy link
Contributor

Urelatert, men på sikt bør vi kanskje skrive om accordion til å bruke: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/details

Da får vi mye gratis og kan droppe hele collapse dependency så vidt jeg forstår hvertfall

@pethel
Copy link
Contributor Author

pethel commented Apr 11, 2024

Urelatert, men på sikt bør vi kanskje skrive om accordion til å bruke: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/details

Da får vi mye gratis og kan droppe hele collapse dependency så vidt jeg forstår hvertfall

Ja kanske det. Jag har satt meg in i den elementtet må jag inrøma. Spørs hvor mye man kan trenge animsjoner. Spørs vell også vad som er lov att ha i en details.

const [overflow, setOverflow] = useState(() =>
isOpen ? 'visible' : 'hidden',
);
const [visibility, setVisibility] = useState(() =>
Copy link
Contributor

@frederikgdl frederikgdl Apr 11, 2024

Choose a reason for hiding this comment

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

Burde kanskje beholde visibility hidden når den er lukket? Da kan ikke elementer fokuseres når den er lukket. Ser det ikke er et problem for accordion for der rendres ikke innholdet hvis lukket

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Det er ett veldig gott poeng. Det skall jag få fikset

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pethel pethel force-pushed the develop_fix-collapse branch from 0f54d79 to 2d3eabc Compare April 11, 2024 13:07
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://black-beach-0d62d0d03-1872.westeurope.2.azurestaticapps.net

@pethel pethel requested a review from frederikgdl April 11, 2024 13:13
className={classNames(
'ffe-collapse',
isOpen && 'ffe-collapse--open',
isHidden && 'ffe-collapse--hidden',
Copy link
Contributor

Choose a reason for hiding this comment

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

Tror syntaksen for classNames er

'ffe-collapse--open': isOpen,
'ffe-collapse--hidden': isHidden 

Copy link
Contributor

@HeleneKassandra HeleneKassandra left a comment

Choose a reason for hiding this comment

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

Bare fiks den classNames syntaksen så er det i orden. Fint å også legge ut en melding på slack hva den breaking changen er, og hva folk må endre

@pethel pethel force-pushed the develop_fix-collapse branch from 2d3eabc to 4ab3f17 Compare April 15, 2024 07:40
@pethel pethel requested a review from HeleneKassandra April 15, 2024 07:41
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://black-beach-0d62d0d03-1872.westeurope.2.azurestaticapps.net

@pethel pethel merged commit 7426fde into develop Apr 15, 2024
3 checks passed
@pethel pethel deleted the develop_fix-collapse branch April 15, 2024 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants