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.

Sign up for GitHub

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

Jump to bottom

PR: AVN-228 nve-alert #15

Merged
merged 12 commits into from
Jan 1, 2024
Merged

PR: AVN-228 nve-alert #15

merged 12 commits into from
Jan 1, 2024

Conversation

amish1188
Copy link
Collaborator

Måtte bruke previeHead i main.ts i storybook konfigurasjon for å kunne laste material-symbols, ellers funka de ikke. PreviewHead overskrevet default import av filer fra index.html derfor måtte jeg legge til alt manuelt. Hvis det finnes en bedre løsning gjerne si fra @joergenlampe .
Lagde nye props i nve-alert som text og title og klarte å injekte tekst gjennom css content property (litt hokus pokus men fungerer fint).

@amish1188 amish1188 changed the title added alert PR: AVN-228 nve-alert Nov 28, 2023
Citaborg
Citaborg reviewed Dec 4, 2023
View reviewed changes
src/main.ts Outdated
</td>
</tr>
</table> `;
<hr />
Copy link
Contributor

Choose a reason for hiding this comment

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

Ikke gyldig html for tabellen rundt button. Fikset i avn225 radio.

@amish1188 amish1188 force-pushed the feature/avn-228/nve-alert branch from f7dcd3e to 50ce0b6 Compare December 11, 2023 14:46
@github-actions GitHub Actions
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://kind-mushroom-03c272603-15.westeurope.3.azurestaticapps.net

@amish1188 amish1188 requested a review from Citaborg December 11, 2023 15:08
@github-actions GitHub Actions
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://kind-mushroom-03c272603-15.westeurope.3.azurestaticapps.net

1 similar comment
@github-actions GitHub Actions
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://kind-mushroom-03c272603-15.westeurope.3.azurestaticapps.net

knutnve
knutnve requested changes Dec 13, 2023
View reviewed changes
Copy link
Collaborator

@knutnve knutnve left a comment

Choose a reason for hiding this comment

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

Kommentar:

Vestre-Ikon er ikke likt selv om det er material symbols?
Close icon er ikke likt. Shoelace ikon?
Forskjellig høyde. Men det styres vel av innholdet + 16px padding?
Bodyteksten oppleves som fetere i kode enn design

alert comment

@amish1188 amish1188 force-pushed the feature/avn-228/nve-alert branch from 44a097e to 1c6f36f Compare December 14, 2023 11:55
@github-actions GitHub Actions
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://kind-mushroom-03c272603-15.westeurope.3.azurestaticapps.net

1 similar comment
@github-actions GitHub Actions
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://kind-mushroom-03c272603-15.westeurope.3.azurestaticapps.net

@knutnve
Copy link
Collaborator

knutnve commented Dec 18, 2023

Liten hover feil her

Screen.Recording.2023-12-18.at.13.00.09.mov

@knutnve
Copy link
Collaborator

knutnve commented Dec 18, 2023

@amish1188 Er storybook den siste gjeldene? Virker som når jeg kjører feature/avn-228/nve-alert lokalt er det ikke helt oppdatert

@knutnve
Copy link
Collaborator

knutnve commented Dec 18, 2023

  • Vi har ikke varianten primary i settet våres.
Screenshot 2023-12-18 at 13 07 18
  • Neutral har egen tokens farger. Se skisser.

@knutnve knutnve mentioned this pull request Dec 18, 2023
Closed
@knutnve knutnve linked an issue Dec 18, 2023 that may be closed by this pull request
AVN-228 nve-alert Design feedback #46
Closed
@amish1188
Copy link
Collaborator Author

  • Vi har ikke varianten primary i settet våres.
Screenshot 2023-12-18 at 13 07 18 * Neutral har egen tokens farger. Se skisser.

info alert i shoelace er av variant primary, så alle css klasser som vi overskrevet heter alert-primary også selve variant som skal brukes i html skal hete akkurat det samme. dette kan vi ikke overskrive (vi kunne men det er masse jobb som kanskje ikke er verdt det). kan vi ikke bare justere ds vårt så at den bruker samme navngivning på alert variantene som shoelace?

@knutnve
Copy link
Collaborator

knutnve commented Dec 19, 2023

  • Vi har ikke varianten primary i settet våres.
Screenshot 2023-12-18 at 13 07 18 * Neutral har egen tokens farger. Se skisser.

info alert i shoelace er av variant primary, så alle css klasser som vi overskrevet heter alert-primary også selve variant som skal brukes i html skal hete akkurat det samme. dette kan vi ikke overskrive (vi kunne men det er masse jobb som kanskje ikke er verdt det). kan vi ikke bare justere ds vårt så at den bruker samme navngivning på alert variantene som shoelace?

Ja det kan vi. Hva om Neutral fargen blir primary da?

@amish1188 amish1188 force-pushed the feature/avn-228/nve-alert branch from 6a06f0b to bf9de32 Compare December 21, 2023 09:03
@amish1188 amish1188 requested a review from knutnve December 21, 2023 09:04
@github-actions GitHub Actions
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://kind-mushroom-03c272603-15.westeurope.3.azurestaticapps.net

knutnve
knutnve approved these changes Dec 21, 2023
View reviewed changes
Copy link
Collaborator

@knutnve knutnve left a comment

Choose a reason for hiding this comment

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

Ser bra ut. Bra jobba. 👍

@amish1188 amish1188 force-pushed the feature/avn-228/nve-alert branch from bf9de32 to bdef761 Compare December 21, 2023 13:36
@github-actions GitHub Actions
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://kind-mushroom-03c272603-15.westeurope.3.azurestaticapps.net

@amish1188
added alert
d1550d3 
added docs

added stylesheet

added styles to storybook head
@github-actions GitHub Actions
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://kind-mushroom-03c272603-15.westeurope.3.azurestaticapps.net

@amish1188 amish1188 force-pushed the feature/avn-228/nve-alert branch from 3f92f8a to 5ce3698 Compare December 21, 2023 15:48
@github-actions GitHub Actions
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://kind-mushroom-03c272603-15.westeurope.3.azurestaticapps.net

2 similar comments
@github-actions GitHub Actions
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://kind-mushroom-03c272603-15.westeurope.3.azurestaticapps.net

@github-actions GitHub Actions
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://kind-mushroom-03c272603-15.westeurope.3.azurestaticapps.net

@amish1188 amish1188 force-pushed the feature/avn-228/nve-alert branch from 3cde195 to 5ce3698 Compare January 1, 2024 17:54
@amish1188 amish1188 merged commit d482b54 into main Jan 1, 2024
3 of 4 checks passed
@amish1188 amish1188 deleted the feature/avn-228/nve-alert branch January 1, 2024 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@knutnve knutnve knutnve approved these changes

@Citaborg Citaborg Citaborg approved these changes

@joergenlampe joergenlampe Awaiting requested review from joergenlampe

@gruble gruble Awaiting requested review from gruble

@elnobanve elnobanve Awaiting requested review from elnobanve

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AVN-228 nve-alert Design feedback
4 participants
@amish1188 @knutnve @Citaborg @joergenlampe