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: UIG-2910 - vl-table-next - introductie component #371

Open
wants to merge 1 commit into
base: develop-v1.42
Choose a base branch
from

Conversation

Goldflow
Copy link
Collaborator

@Goldflow Goldflow commented Nov 7, 2024

Storybook verbeterd, cypress uitgebreid & meta-data aangepast.

Jira
Bamboo

@Goldflow Goldflow force-pushed the feature/UIG-2910-data-table-to-lit branch from 9227f18 to c333067 Compare November 7, 2024 14:54
@Goldflow Goldflow changed the title feat: UIG-2910 - vl-data-table-next - introductie component feat: UIG-2910 - vl-table-next - introductie component Nov 7, 2024
@Goldflow Goldflow force-pushed the feature/UIG-2910-data-table-to-lit branch from c333067 to 209ff87 Compare November 7, 2024 14:58
Storybook verbeterd, cypress uitgebreid & meta-data aangepast.
@Goldflow Goldflow force-pushed the feature/UIG-2910-data-table-to-lit branch from 209ff87 to 3c134d4 Compare November 8, 2024 10:03
@Goldflow Goldflow marked this pull request as ready for review November 8, 2024 11:18
@Goldflow Goldflow requested a review from kspeltix as a code owner November 8, 2024 11:18
Copy link
Member

@kspeltix kspeltix left a comment

Choose a reason for hiding this comment

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

Je mag hem rebasen tov develop en als PR tov develop aanbieden. Prioriteit is de text maar als deze klaar zou zijn voor release van 1.42 kan hij nog mee, anders wordt het 1.43

@@ -11,7 +11,7 @@ import * as VlSearchFilterStories from './vl-search-filter.stories';
<VluxAlert type="info">
{`
In de v2 versie van deze component gebruik je hem via de custom-tag, de interne implementatie is voor de rest gelijk gebleven aan deze van de v1 versie.
In v3 zal deze component grondig herwerkt worden; in de context van een herwerking van de vl-data-table.
In v3 zal deze component grondig herwerkt worden; in de context van een herwerking van de vl-table.
Copy link
Member

Choose a reason for hiding this comment

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

In zou niet over v3 spreken, ik zou eerder zeggen: In de toekomst

matrix: {
name: 'matrix',
description:
'Attribuut wordt gebruikt om data in 2 dimensies te tonen. Zowel de rijen als de kolommen krijgen een titel. Deze titels worden gescheiden door een dikke lijn.',
Copy link
Member

Choose a reason for hiding this comment

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

max 120 characters en dan een enter

<VluxAlert type="info">
{`
In de v2 versie van deze component gebruik je hem via de custom-tag, de interne implementatie is voor de rest gelijk gebleven aan deze van de v1 versie.
In v3 zal deze component grondig herwerkt worden; in de context van een herwerking van de vl-table.
Copy link
Member

Choose a reason for hiding this comment

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

In de toekomst ...

</thead>
<tbody>
<tr>
<td data-title="Entry Header 1">Entry line 1</td>
Copy link
Member

Choose a reason for hiding this comment

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

data-title, het lijkt mij niet dat er een data-title attribuut bestaat

<thead>
<tr>
<th>Name</th>

Copy link
Member

Choose a reason for hiding this comment

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

al die blanco lijnen maken het vooral meer ruimte innemen

return this.querySelector('caption');
}

private get _headHeaderElements(): HTMLTableCellElement[] {
Copy link
Member

Choose a reason for hiding this comment

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

private is voldoende, geen _


/* from assets */

.vl-table-next {
Copy link
Member

Choose a reason for hiding this comment

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

nesten met een & waar kan

width: 100%;
max-width: 100%;
}
.vl-table-next caption {
Copy link
Member

Choose a reason for hiding this comment

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

normaal zet je zo graag blanco lijnen

});
});

const mountExpandable = ({
Copy link
Member

Choose a reason for hiding this comment

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

normaal niet, maar in dit geval misschien de te mounten code en utility methodes in een apart bestand ernaast zetten?

@@ -31,8 +31,8 @@ describe('valideer de volledigheid van de gegenereerde web-types', () => {
expect(elementWTWithoutWC).toStrictEqual([]);
});
it('components - valideer de volledigheid van de web-types', () => {
expect(componentWCNameCount).toEqual(92);
expect(componentWTNameCount).toEqual(73);
expect(componentWCNameCount).toEqual(93);
Copy link
Member

Choose a reason for hiding this comment

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

als je rebased tov develop zal dit ook aangepast moeten worden

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.

2 participants