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

Implementation of Toolbar component #6405

Merged
merged 22 commits into from
May 24, 2024
Merged

Implementation of Toolbar component #6405

merged 22 commits into from
May 24, 2024

Conversation

chrisalat
Copy link
Member

@chrisalat chrisalat commented May 8, 2024 •
edited by sdvg

☑️ Definition of Done checklist

  • Meaningful title for the release notes
  • Pull request is linked to a problem
  • All changes relate to the problem
  • A11y tests performed successfully or not relevant
  • Manual test performed successfully (by reviewer) or not relevant

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented May 8, 2024 •
edited

Netlify Draft Deployment
URL: https://6650be90c52172171cce1d68--kolibri-public-ui.netlify.app
Logs: https://app.netlify.com/sites/kolibri-public-ui/deploys/6650be90c52172171cce1d68

@deleonio deleonio force-pushed the develop branch 2 times, most recently from 487bb0f to f56f225 Compare May 14, 2024 02:40
deleonio
deleonio requested changes May 14, 2024
View reviewed changes
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.

Im Großen und Ganzen schon ganz gut.

Jetzt müssten wir das Szenario noch weiter für reale Szenarien ausbauen.

packages/components/src/components/toolbar/constants.ts Outdated Show resolved Hide resolved
deleonio
deleonio requested changes May 14, 2024
View reviewed changes
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.

Suche bitte mal die stencil.config.js dort muss noch die neue Toolbar-Komponente eingetragen werden.

https://github.com/public-ui/kolibri/blob/5728-toolbar-component/packages/components/stencil.config.ts

packages/components/src/components/toolbar/readme.md Outdated Show resolved Hide resolved
@chrisalat chrisalat linked an issue May 16, 2024 that may be closed by this pull request
Component: Toolbar neu umsetzen #5728
Closed
9 tasks
@chrisalat chrisalat marked this pull request as ready for review May 22, 2024 14:46
@chrisalat
Copy link
Member Author

Vielleicht noch zwei Anmerkungen:

  1. Sollen wir die Links im BMF-Theme weiß gestalten?
  2. Lassen wir aria-controls erstmal weg oder sollen wir die noch umsetzen? Falls ja, über eine id gesteuert oder direkt als Attribut _ariaControls?

@chrisalat chrisalat requested a review from deleonio May 22, 2024 14:53
@deleonio
Merge branch 'develop' of https://github.com/public-ui/kolibri into 5…
cfdeaa9 
…728-toolbar-component
@deleonio
Update readme.md
5e5c949 
Signed-off-by: Martin <6279703+deleonio@users.noreply.github.com>
@sdvg
Copy link
Member

sdvg commented May 24, 2024

@chrisalat Ich würde die neuen Test-Routen gerne noch für das visuelle Testing aufnehmen. Siehe packages/tools/visual-tests/tests/sample-app.routes.js. Dort eintragen und anschließend Snapshots erstellen.

@sdvg
Copy link
Member

sdvg commented May 24, 2024

@chrisalat Das disabled-Beispiel scheint mir nicht ganz richtig zu funktionieren:

image

Hier sind die Items 1 und 3 aktiv. Wenn ich 1 aktiviert habe, würde ich davon ausgehen, dass ich mit einmal Pfeiltaste rechts drücken zu Item 3 komme. Aktuell muss ich zwei mal drücken.

sdvg
sdvg requested changes May 24, 2024
View reviewed changes
packages/components/src/components/toolbar/shadow.tsx Outdated Show resolved Hide resolved
packages/components/src/components/toolbar/shadow.tsx Outdated Show resolved Hide resolved
packages/components/src/components/toolbar/shadow.tsx Outdated Show resolved Hide resolved
packages/components/src/components/toolbar/shadow.tsx Outdated Show resolved Hide resolved
packages/components/src/components/toolbar/shadow.tsx Outdated Show resolved Hide resolved
packages/components/src/components/toolbar/shadow.tsx Outdated Show resolved Hide resolved
@chrisalat
Copy link
Member Author

@sdvg Laut dem Beispiel von W3C müssen die disabled Elemente trotzdem fokussiert werden. Meine erste Logik war tatsächlich dafür gedacht die Element zu überspringen. Hab ich aber dann wieder entfernt :)

@chrisalat Das disabled-Beispiel scheint mir nicht ganz richtig zu funktionieren:

image Hier sind die Items 1 und 3 aktiv. Wenn ich 1 aktiviert habe, würde ich davon ausgehen, dass ich mit einmal `Pfeiltaste rechts` drücken zu Item 3 komme. Aktuell muss ich zwei mal drücken.

@chrisalat chrisalat requested review from sdvg and deleonio May 24, 2024 12:17
sdvg added 2 commits May 24, 2024 15:08
@sdvg
Replace DOM access with element ref map
216f742 
Refs: #5728
@sdvg
Merge remote-tracking branch 'origin/5728-toolbar-component' into 572…
dc9cebe 
…8-toolbar-component

* origin/5728-toolbar-component:
  (#5728) Review changes
sdvg
sdvg previously approved these changes May 24, 2024
View reviewed changes
sdvg
sdvg previously approved these changes May 24, 2024
View reviewed changes
@deleonio deleonio merged commit 3ae500c into develop May 24, 2024
@deleonio deleonio deleted the 5728-toolbar-component branch May 24, 2024 17:13
@github-actions github-actions bot locked and limited conversation to collaborators May 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@deleonio deleonio deleonio approved these changes

@sdvg sdvg sdvg left review comments

Assignees
No one assigned
Labels
re-review v2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Component: Toolbar neu umsetzen
3 participants
@chrisalat @sdvg @deleonio