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 table column consistency #7047

Merged
merged 24 commits into from
Nov 27, 2024
Merged

Fix table column consistency #7047

merged 24 commits into from
Nov 27, 2024

Conversation

anicyne
Copy link
Contributor

@anicyne anicyne commented Nov 8, 2024

Refs: #6890

The A11y and PO reviews will only take place after all other DoD steps have been completed by the Developer:

  • Meaningful pull request title for the release notes
  • Pull request is linked to an issue
  • All changes relate to the issue
  • No TODOs or commented out code in the final commit
  • Tests to protect this code implemented (if applicable)
  • Manual test performed successfully (if applicable)
  • Documentation or migration has been updated (if applicable)

@anicyne anicyne linked an issue Nov 8, 2024 that may be closed by this pull request
Copy link
Contributor

github-actions bot commented Nov 8, 2024

@deleonio deleonio added v3 Here are issues that need to be resolved for version 3. v2 and removed v3 Here are issues that need to be resolved for version 3. labels Nov 12, 2024
@deleonio
Copy link
Contributor

deleonio commented Nov 14, 2024

Hi @sdvg, @AlexanderSchmidtCE ,

irgendwie ist der Snapshot jetzt anders ... die Tabelle ist nicht mehr leer?

Wurde das Sample gefixed oder die Component?

Müsste das nicht https://public-ui.github.io/v2/sample-react/#/table/stateless fixen?

@sdvg
Copy link
Member

sdvg commented Nov 14, 2024

irgendwie ist der Snapshot jetzt anders ... die Tabelle ist nicht mehr leer?

Hier ist ein kleiner Abstand entstanden. Das sollte noch behoben werden.

screenshot 2024-11-14-11 35 53@2x

Wurde das Sample gefixed oder die Component?

Es wurde direkt in der Tabelle gefixt. Reviewed ist die Änderung noch nicht.

Müsste das nicht public-ui.github.io/v2/sample-react#/table/stateless fixen?

Um dieses Beispiel geht es im Ticket, ja. Da wurden jetzt leere Zellen eingefügt, wo vorher keine waren.

@deleonio deleonio changed the base branch from develop to release/2 November 14, 2024 15:28
@anicyne anicyne force-pushed the 6890-fix-table-column branch from b94b182 to a910ae5 Compare November 20, 2024 12:39
@anicyne anicyne force-pushed the 6890-fix-table-column branch from a910ae5 to bf96960 Compare November 20, 2024 12:40
@deleonio deleonio requested a review from sdvg November 20, 2024 13:51
@deleonio
Copy link
Contributor

@sdvg ist das eher so, wie Du wolltest?

@sdvg
Copy link
Member

sdvg commented Nov 20, 2024

@sdvg ist das eher so, wie Du wolltest?

Noch nicht so ganz:

Im Beispiel sind weiterhin die Spalten verschoben.
screenshot 2024-11-20-18 02 20@2x

Das ließe sich lösen, indem wir eine leere Headerspalte als Platzhalter für die vertikalen Header hinzufügen.
Dies würde wie folgt aussehen:

_headerCells={{
	horizontal: [
		[
+			{ label: '', asTd: true },
			{ key: 'left', label: 'left', textAlign: 'left', sortDirection: 'ASC' },
			{ key: 'center', label: 'center', textAlign: 'center', sortDirection: 'DESC' },
			{ key: 'right', label: 'right', textAlign: 'right', sortDirection: 'NOS' },
			{ key: 'nosort', label: 'no sort option' },
		],
	],

Wenn die Tabelle korrekt befüllt ist, ist es auch nicht mehr notwendig, Daten "abzuschneiden", die über die Header-Spalten herausragen, wie gerade implementiert. Diese Logik würde ich tatsächlich lieber wieder entfernen, weil ich es im Zweifelsfall besser fände, wenn Daten und Header nicht ganz zusammenpassen, als wenn Daten entfernt werden.

Für mich wäre eine solche Fehlerbehebung direkt im Beispiel ausreichend. Wenn wir zusätzlich Probleme in der Tabelle selbst abfangen wollen, würde ich gerne noch einmal über die Details sprechen (@deleonio).

@anicyne anicyne force-pushed the 6890-fix-table-column branch from 875b66d to 16c041f Compare November 21, 2024 13:47
@anicyne anicyne requested a review from deleonio November 22, 2024 12:16
Copy link
Contributor

@deleonio deleonio left a comment

Choose a reason for hiding this comment

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

@sdvg Ich glaube, hier ist aus Versehen ein Fehler passiert. v3stuff containt

@sdvg sdvg force-pushed the 6890-fix-table-column branch from e8cb849 to 332cad6 Compare November 23, 2024 10:31
@sdvg
Copy link
Member

sdvg commented Nov 23, 2024

@sdvg Ich glaube, hier ist aus Versehen ein Fehler passiert. v3stuff containt

Danke für den Hinweis. Da war ich gestern noch auf dem falschen Branch unterwegs. Ist korrigiert.

deleonio

This comment was marked as spam.

@deleonio deleonio requested review from deleonio and sdvg November 23, 2024 23:28
deleonio

This comment was marked as spam.

@deleonio deleonio requested review from deleonio and removed request for deleonio November 25, 2024 12:31
Copy link
Member

@sdvg sdvg left a comment

Choose a reason for hiding this comment

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

@deleonio Die Lösung ist nachvollziehbar und sieht für mich gut aus. Die neuen Unit-Tests finde ich super, die sollten auf jeden Fall helfen, die Render-Logik zu stabilisieren.

Ich habe ein paar Kleinigkeiten kommentiert, die mir unterwegs aufgefallen sind.

@deleonio deleonio requested a review from sdvg November 26, 2024 07:25
@sdvg sdvg merged commit 42f5ca9 into release/2 Nov 27, 2024
8 checks passed
@sdvg sdvg deleted the 6890-fix-table-column branch November 27, 2024 13:13
@github-actions github-actions bot locked and limited conversation to collaborators Nov 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9.1.3.1e - table (stateless): Datentabelle richtig aufgebaut
3 participants