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

feat: add StudioSearch component #14348

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

standeren
Copy link
Contributor

@standeren standeren commented Jan 3, 2025

Description

  • add StudioSearch component.
  • use new component in code list page in library.
  • use new component in text editor.
  • use new compone

Dashboard:
Skjermbilde 2025-01-06 kl  13 48 12
nt on dashboard.

Text-editor:
Skjermbilde 2025-01-06 kl  13 49 00

CodeLists in content library:
Skjermbilde 2025-01-06 kl  13 49 19

Related Issue(s)

Verification

  • Your code builds clean without any errors or warnings
  • Manual testing done (required)

@github-actions github-actions bot added area/text-editor Area: Related to creating, translating and editing texts. solution/studio/designer Issues related to the Altinn Studio Designer solution. frontend labels Jan 3, 2025
@standeren standeren added skip-releasenotes Issues that do not make sense to list in our release notes area/content-library Area: Related to library for shared resources team/studio-domain1 skip-documentation Issues where updating documentation is not relevant labels Jan 3, 2025
@TomasEng TomasEng self-assigned this Jan 3, 2025
Copy link
Contributor

Choose a reason for hiding this comment

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

Jeg ser at det er gjort på denne måten enkelte andre steder, men for å følge fasademønsteret, bør vi egentlig lage en egen komponent rundt designsystemkomponenten. Da kan vi også sette size='sm' som standard og ta i bruk WithoutAsChild på prop-typen.

@TomasEng TomasEng assigned standeren and unassigned TomasEng Jan 3, 2025
@ErlingHauan
Copy link
Contributor

Vi har et søkefelt på dashbordet også. Hvis det ikke er for mye jobb, kan du legge den til der i tillegg?

@github-actions github-actions bot added the area/dashboard Area: Related to the dashboard application label Jan 3, 2025
@github-actions github-actions bot added the quality/testing Tests that are missing, needs to be created or could be improved. label Jan 6, 2025
Copy link

codecov bot commented Jan 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.54%. Comparing base (1e059fd) to head (f2cc3c9).

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #14348   +/-   ##
=======================================
  Coverage   95.54%   95.54%           
=======================================
  Files        1862     1864    +2     
  Lines       24125    24134    +9     
  Branches     2785     2785           
=======================================
+ Hits        23050    23060   +10     
  Misses        817      817           
+ Partials      258      257    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@standeren standeren assigned TomasEng and unassigned standeren Jan 6, 2025
@TomasEng TomasEng self-requested a review January 6, 2025 08:34
@TomasEng TomasEng assigned standeren and unassigned TomasEng Jan 6, 2025
@standeren standeren removed their assignment Jan 6, 2025
@standeren standeren force-pushed the add-studio-component-for-search branch from 50c0a82 to 2679d7c Compare January 6, 2025 14:17
@TomasEng TomasEng removed their assignment Jan 7, 2025
@TomasEng TomasEng self-requested a review January 7, 2025 06:10
Copy link
Contributor

@TomasEng TomasEng left a comment

Choose a reason for hiding this comment

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

Var litt rask med å godkjenne denne. Se kommentar.

Comment on lines 13 to 14
<Label>{label}</Label>
<Search {...rest} size={size} ref={ref} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Når vi bruker label, må vi bruke htmlFor til å referere til riktig felt. Dette refererer til HTML-attributtet for. ID-en kan vi generere ved hjelp av useId.

Suggested change
<Label>{label}</Label>
<Search {...rest} size={size} ref={ref} />
<Label htmlFor={id}>{label}</Label>
<Search {...rest} id={id} size={size} ref={ref} />

Dette kan vi verifisere med en test som bruker screen.getByRole('searchbox', { name: label }) til å finne søkefeltet.

For øvrig lurer jeg på hva som er hensikten med StudioFieldset her?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Skal fikse!

StudiofieldSet la jeg til fordi DS hadde dette kodeeksempelet bare. Men jeg er på tynn is her, så godt mulig det ikke er riktig! https://storybook.designsystemet.no/iframe.html?viewMode=docs&id=komponenter-search--docs&globals=#med-label

Copy link
Contributor

Choose a reason for hiding this comment

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

I dette eksemplet bruker de Field, ikke Fieldset. Jeg går ut fra at dette er noe som er lagt til versjon 1 for å fikse dette med ID automatisk.

@standeren standeren assigned TomasEng and unassigned standeren Jan 7, 2025
return showLabel ? (
<StudioFieldset legend={legend}>
<Label htmlFor='searchId'>{label}</Label>
<Search {...rest} id='searchId' size={size} ref={ref} />
Copy link
Contributor

@TomasEng TomasEng Jan 7, 2025

Choose a reason for hiding this comment

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

Vi bør ikke hardkode ID-en. En ID må være unik, så dette vil gjøre DOM-en ugyldig hvis siden inneholder flere søkefelter. Derfor bør vi generere en unik ID med useId. Vi bør også sjekke om det allerede er satt in ID i proppene. I så fall er det den som skal gjelde.

testCustomAttributes(renderTestSearch, getSearchBox);
});

it('should find search component with label name', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
it('should find search component with label name', () => {
it('should render search field with label name', () => {

@TomasEng TomasEng assigned standeren and unassigned TomasEng Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/content-library Area: Related to library for shared resources area/dashboard Area: Related to the dashboard application area/text-editor Area: Related to creating, translating and editing texts. frontend quality/testing Tests that are missing, needs to be created or could be improved. skip-documentation Issues where updating documentation is not relevant skip-releasenotes Issues that do not make sense to list in our release notes solution/studio/designer Issues related to the Altinn Studio Designer solution. team/studio-domain1
Projects
Status: 👷 In Progress
Development

Successfully merging this pull request may close these issues.

3 participants