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

HLD: Handling table cell state when scrolling, cell custom element changes #1052

Merged
merged 21 commits into from
Mar 23, 2023
Merged

HLD: Handling table cell state when scrolling, cell custom element changes #1052

merged 21 commits into from
Mar 23, 2023

Conversation

msmithNI
Copy link
Contributor

@msmithNI msmithNI commented Feb 13, 2023 •
edited
Loading

Pull Request

🤨 Rationale

HLD for #997

Note: This HLD/PR also has some new proposed changes dealing with using custom elements provided by the column plugin authors in table cells.

👩‍💻 Implementation

N/A

🧪 Testing

Research done locally in Chrome, Firefox, and Safari.

See prototypes linked in the HLD.

✅ Checklist

  • I have updated the project documentation to reflect my changes or determined no changes are needed.

@msmithNI msmithNI marked this pull request as ready for review February 13, 2023 22:45
@nate-ni nate-ni mentioned this pull request Feb 14, 2023
Closed
@msmithNI
Copy link
Contributor Author

msmithNI commented Feb 23, 2023 •
edited
Loading

@rajsite updated the HLD with the proposal we talked about.
@jattasNI @mollykreis Milan floated another idea of raising an event on the cell/content container that can be handled from the column implementor's template, instead of the original method proposed (onBeforeFocusedCellRecycled).
I reworked the HLD to cover both of those ideas, so let me know if you have a preference.

I also did some prototyping around the action menu to come up with some code to handle closing those, so updated that part of the HLD.

I also moved all of the text selection info to "Alternative Implementations" since based on previous discussions, we're not planning on doing anything for text selection.

I also have a Storybook of my prototype if anyone wants to dig into the implementation details more (I have some console.log statements if you scroll with a focused cell / open action menu that should make it easier to jump to the related code in DevTools).

jattasNI
jattasNI approved these changes Feb 27, 2023
View reviewed changes
Copy link
Contributor

@jattasNI jattasNI left a comment

Choose a reason for hiding this comment

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

I don't have an opinion on option 1 vs 2 but let me know if you need help making that decision.

packages/nimble-components/src/table/specs/cell-state-when-scrolling-hld.md Outdated Show resolved Hide resolved
packages/nimble-components/src/table/specs/cell-state-when-scrolling-hld.md Show resolved Hide resolved
@rajsite rajsite mentioned this pull request Mar 3, 2023
Merged
1 task
@msmithNI msmithNI changed the title HLD: Handling table cell state when scrolling HLD: Handling table cell state when scrolling, cell custom element changes Mar 10, 2023
@msmithNI
Copy link
Contributor Author

@rajsite @atmgrifter00 @jattasNI @mollykreis this should be ready to re-review now, with the custom element changes we discussed. There'll probably need some more changes needed to naming of things like cellViewTag and BaseCellElement but I think it's ready for another round of feedback at least.

@msmithNI
Copy link
Contributor Author

@rajsite addressed the open issues on this HLD. Keeping it open for now since you haven't approved it yet / in case you want to take a look again.

@msmithNI msmithNI merged commit 83c21ce into main Mar 23, 2023
@msmithNI msmithNI deleted the table-cell-state-scrolling-hld branch March 23, 2023 21:26
@msmithNI msmithNI mentioned this pull request Mar 28, 2023
Merged
1 task
msmithNI added a commit that referenced this pull request Apr 4, 2023
@msmithNI
Nimble Table: cell view (custom element) and cell state changes (#1138)
91172d9 
# Pull Request

## 🤨 Rationale

Fixes #997 

## 👩‍💻 Implementation

Implementation is similar to what was outlined in the the
[HLD](https://github.com/ni/nimble/blob/main/packages/nimble-components/src/table/specs/cell-state-when-scrolling-hld.md)
([PR](#1052)):
- Add a `TableCellView<TCellRecord, TColumnConfig>` class. TableCellView
instances will be used in `TableCell`s, instead of the old approach
(column-provided cellTemplate and cellStyles).
- Add `cellViewTag` property to `TableColumn` (links the column with the
associated `TableCellView` type)
- `TableCellView` defines a `focusedRecycleCallback()` method which is a
no-op by default. If a cell view contains a focusable element, and a
cell is focused, and then a scroll (row recycle) occurs, this gives the
cell view an opportunity to commit/blur/etc.
- We don't yet have a column type exercising this, but there's a new
autotest for this code path.
- If a cell action menu is open when a scroll/ row recycle occurs, the
action menu is closed (via the associated menu button).

## 🧪 Testing
- Updated existing autotests
- Added new tests for `focusedRecycleCallback` and the code that closes
action menus on scroll
- Manually tested table in Storybook + Angular + Blazor example apps

## ✅ Checklist

- [x] I have updated the project documentation to reflect my changes or
determined no changes are needed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rajsite rajsite rajsite left review comments

@atmgrifter00 atmgrifter00 atmgrifter00 approved these changes

@jattasNI jattasNI jattasNI approved these changes

@mollykreis mollykreis mollykreis approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@msmithNI @rajsite @jattasNI @mollykreis @atmgrifter00