Add Ok Breakpoint Column#2963
Conversation
|
@hellovolcano, Here's @msmithNI 's innovation day branch for an ok custom column type (aka a square breakpoint column 🟥): main...users/masmith/innovationDay-okTableColumn1 I haven't looked at either implementation but it could be useful to scan it for differences with your approach and ping him with any questions. (Note when we talked I misspoke about who had done this project, it is definitely Malcolm) |
|
FYI @jattasNI -- I know we originally talked about adding a label provider for ok. I'm open to adding that later, but for this first review I just used stringsin the component itself. |
jattasNI
left a comment
jattasNI
left a comment
There was a problem hiding this comment.
Reviewed the ok-components changes with some rigor (not quite normal Nimble level but more than normal Ok level). Only just skimmed the Blazor changes.
Most of the feedback is nice-to-have for the Ok level with the exception of the tab focus getting stuck on the breakpoint.
Co-authored-by: Jesse Attas <jattasNI@users.noreply.github.com>
jattasNI
left a comment
jattasNI
left a comment
There was a problem hiding this comment.
On this pass I mostly reviewed for Storybook and "ok" level code/testing.
| menuSlot: { | ||
| name: 'menu-slot', | ||
| description: | ||
| 'The name of the slot within the table where context menu content is provided. When configured, context menu requests render this slotted content inside an anchored region in the active breakpoint cell.', | ||
| control: false, | ||
| table: { category: apiCategory.attributes } | ||
| }, | ||
| toggleEvent: { | ||
| name: 'breakpoint-column-toggle', | ||
| description: | ||
| 'Emitted when a breakpoint is toggled via click or keyboard. The event detail includes `recordId`, `oldState`, and `newState`.', | ||
| control: false, | ||
| table: { category: apiCategory.events } | ||
| }, | ||
| contextMenuEvent: { | ||
| name: 'breakpoint-column-context-menu', | ||
| description: | ||
| 'Emitted when the breakpoint context menu is requested. The event detail includes `recordId` and `currentState`.', | ||
| control: false, | ||
| table: { category: apiCategory.events } | ||
| }, |
There was a problem hiding this comment.
With the new approach where the menu is provided in a slot I'm less clear about the purpose of the context menu event callback. It might be helpful to clarify what you can / must do in the callback. Do you need to show the menu? Use the opportunity to populate the menu contents?
| @@ -0,0 +1,87 @@ | |||
| import type { TablePageObject } from '@ni/nimble-components/dist/esm/table/testing/table.pageobject'; | |||
There was a problem hiding this comment.
Page objects typically go in /testing rather than /tests which allows them to be exported and used by clients
| }); | ||
| }); | ||
|
|
||
| describe('context menu', () => { |
There was a problem hiding this comment.
I'm only seeing asserts about the events getting fired, not the state of the context menu itself. It's possible I'm missing something and it's up to you how much test coverage you want.
3 participants
Pull Request
🤨 Rationale
A client needs a breakpoint column for debugging workflows.
HLD + IxD
👩💻 Implementation
This component mimics the nimble menu button component, but in addition to the toggle events on click, also includes a context menu event to allow clients to set additional states.
Added:
🧪 Testing
manual testing
added unit tests
✅ Checklist