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

fix: Align the main headings on the data model page #13786

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

Conversation

ErlingHauan
Copy link
Contributor

@ErlingHauan ErlingHauan commented Oct 14, 2024

Description

Changes:

  • Modified CSS to make the three column headings in Datamodell sit on the same line.
  • Used StudioHeading for the headings.
  • Slight refactor of SchemaInspector, in order to place both return statements in the bottom of the component.
  • Removed the collapsed prop in the resizable layout that wraps around SchemaInspector, so that the width of the column is the same width, regardless if a node is selected.
  • Removed unused code.

Before

before-unselected-node
before-selected-node
before-type-view

After

after-unselected-node
after-selected-node
after-type-view

Verification

  • Your code builds clean without any errors or warnings
  • Manual testing done (required)
  • Relevant automated test added (if you find this hard, leave it and we'll help out)

@github-actions github-actions bot added area/data-modeling Area: Related to data models - e.g. create, edit, use data models. solution/studio/designer Issues related to the Altinn Studio Designer solution. frontend labels Oct 14, 2024
@ErlingHauan ErlingHauan self-assigned this Oct 15, 2024
Copy link

codecov bot commented Oct 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.57%. Comparing base (74019c5) to head (97f9054).
Report is 23 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #13786      +/-   ##
==========================================
- Coverage   94.57%   94.57%   -0.01%     
==========================================
  Files        1624     1624              
  Lines       21817    21813       -4     
  Branches     2572     2572              
==========================================
- Hits        20634    20630       -4     
+ Misses        940      939       -1     
- Partials      243      244       +1     

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

@ErlingHauan ErlingHauan changed the title chore: Align the main headings on the data model page style: Align the main headings on the data model page Oct 15, 2024
@ErlingHauan ErlingHauan marked this pull request as ready for review October 17, 2024 12:36
@ErlingHauan ErlingHauan removed their assignment Oct 17, 2024
@TomasEng TomasEng self-assigned this Oct 18, 2024
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.

Alt i alt fin opprydning!

Hvis vi skal følge konvensjonen for endringstitler, viser style til endringer i kodeformat. fix er nok mer passende her.

Comment on lines 14 to 20
border-bottom-left-radius: 0;
border-left-color: transparent;
border-left-style: solid;
border-left-width: var(--studio-treeitem-vertical-line-width);
border-top-left-radius: 0;
display: flex;
font: var(--fds-typography-paragraph-short-large);
min-height: var(--fds-sizing-12);
Copy link
Contributor

Choose a reason for hiding this comment

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

Disse egenskapene er her for å skape en likhet mellom rotelementet og underelementene når de er valgt (se bilder). Hva skjer når du tar dette bort?

image
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oi, det var ikke meningen. Bra du fanget det opp!

Ser at dette må være tilstede:

display: flex;
font: var(--fds-typography-paragraph-short-large);
border-left-width: var(--studio-treeitem-vertical-line-width);
border-top-left-radius: 0;
border-bottom-left-radius: 0;

Mens det virker som om dette ikke har noen funksjon:

border-left-color: transparent;
border-left-style: solid;

Bør være greit å ta vekk, eller?

Copy link
Contributor

Choose a reason for hiding this comment

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

Jeg tror faktisk de to linjene også må være der. Når det står border-color: transparent, er det ofte for å hindre at ting flytter på seg når konturen blir borte eller dukker opp. Her skjer det når brukeren bytter mellom valgt / ikke valgt.

Eventuelt kunne vi valgt en helt annen fremgangsmåte her, f.eks. med background med linear-gradient eller med box-shadow. Da kan vi unngå denne litt uheldige effekten av å bruke border:
image

{!isDataModelRoot && <BackButton />}
<HeadingRow schemaPointer={schemaPointer} />
<HeadingRow schemaPointer={schemaPointer} />
{!isDataModelRoot && <BackButton />}
Copy link
Contributor

Choose a reason for hiding this comment

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

Jeg er usikker på om vi burde plassere <BackButton> under overskriften. Blir det semantisk riktig? Dette er jo ikke innhold som er spesifikt for akkurat denne overskriften.

Comment on lines 45 to 55
it('displays a message when no node is selected', () => {
renderSchemaInspector(mockUiSchema);

const noSelectionHeader = screen.getByRole('heading', {
name: textMock('schema_editor.properties'),
});
const noSelectionParagraph = screen.getByText(textMock('schema_editor.no_item_selected'));

expect(noSelectionHeader).toBeInTheDocument();
expect(noSelectionParagraph).toBeInTheDocument();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Fin test! Synes bare variablenavnet noSelectionHeader kan virke litt forvirrende, siden den bare sier "Egenskaper". Hva med propertiesHeader? Dette er småpirk.

Comment on lines 56 to 58
<Tabs.Content value={t('schema_editor.fields')}>
<Alert severity='info'>{t('app_data_modelling.fields_information')}</Alert>
</Tabs.Content>
Copy link
Contributor

Choose a reason for hiding this comment

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

Fint at du fikk plassert denne i en <Tabs.Content>! Kanskje du også kan flytte inn hele shouldDisplayFieldsTab-sjekken, siden det ikke er noen forskjell på de to <Tabs.Content>-ene?

@TomasEng TomasEng assigned ErlingHauan and unassigned TomasEng Oct 21, 2024
@ErlingHauan ErlingHauan changed the title style: Align the main headings on the data model page fix: Align the main headings on the data model page Oct 22, 2024
@@ -171,3 +177,13 @@
</StudioDeleteButton>
);
};

const BackButton = () => (

Check notice

Code scanning / CodeQL

Unused variable, import, function or class Note

Unused variable BackButton.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
added-to-sprint area/data-modeling Area: Related to data models - e.g. create, edit, use data models. frontend solution/studio/designer Issues related to the Altinn Studio Designer solution. team/studio-domain1 ux UX help needed
Projects
Status: 👷 In Progress
Development

Successfully merging this pull request may close these issues.

2 participants