-
Notifications
You must be signed in to change notification settings - Fork 8
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
Add cell placeholders #1950
Merged
Merged
Add cell placeholders #1950
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
@m-akinc, will you buddy this PR for me? |
m-akinc
reviewed
Mar 20, 2024
packages/nimble-components/src/table-column/anchor/tests/table-column-anchor.mdx
Outdated
Show resolved
Hide resolved
packages/nimble-components/src/table-column/anchor/tests/table-column-anchor.mdx
Show resolved
Hide resolved
packages/nimble-components/src/table-column/anchor/tests/table-column-anchor.mdx
Outdated
Show resolved
Hide resolved
packages/nimble-components/src/table-column/anchor/tests/table-column-anchor.mdx
Outdated
Show resolved
Hide resolved
packages/nimble-components/src/table-column/anchor/tests/table-column-anchor.spec.ts
Outdated
Show resolved
Hide resolved
packages/nimble-components/src/table-column/text/tests/table-column-text-matrix.stories.ts
Outdated
Show resolved
Hide resolved
packages/nimble-components/src/table-column/text/tests/table-column-text.mdx
Outdated
Show resolved
Hide resolved
packages/nimble-components/src/table-column/text/tests/table-column-text.spec.ts
Show resolved
Hide resolved
packages/nimble-components/src/table-column/text/tests/table-column-text.spec.ts
Show resolved
Hide resolved
packages/nimble-components/src/table-column/text/tests/table-column-text.spec.ts
Outdated
Show resolved
Hide resolved
m-akinc
approved these changes
Mar 21, 2024
jattasNI
reviewed
Mar 22, 2024
packages/nimble-components/src/table-column/date-text/tests/table-column-date-text.mdx
Show resolved
Hide resolved
jattasNI
approved these changes
Mar 25, 2024
rajsite
reviewed
Mar 25, 2024
packages/nimble-components/src/table-column/anchor/tests/table-column-anchor.stories.ts
Outdated
Show resolved
Hide resolved
packages/nimble-components/src/table-column/anchor/tests/table-column-anchor.spec.ts
Outdated
Show resolved
Hide resolved
packages/nimble-components/src/table-column/anchor/tests/table-column-anchor.mdx
Outdated
Show resolved
Hide resolved
packages/nimble-components/src/table-column/number-text/tests/table-column-number-text.spec.ts
Outdated
Show resolved
Hide resolved
...ges/nimble-components/src/table-column/number-text/tests/table-column-number-text.stories.ts
Outdated
Show resolved
Hide resolved
packages/nimble-components/src/table-column/text/tests/table-column-text.spec.ts
Outdated
Show resolved
Hide resolved
packages/nimble-components/src/table-column/text/tests/table-column-text.stories.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Milan Raj <[email protected]>
rajsite
approved these changes
Mar 26, 2024
1 task
jattasNI
pushed a commit
that referenced
this pull request
Mar 27, 2024
# Pull Request ## 🤨 Rationale This came up in [a discussion on the PR to add placeholders to table columns](#1950 (comment)). The goal is the increase pipeline stability and reproducibility by ensuring more of the configuration is consistent regardless of what machine is running the pipeline. In this case, I am setting the time zone to _America/Chicago_. ## 👩💻 Implementation - set `TZ` environment variable to `Americal/Chicago` - set `--time-zone-for-testing=America/Chicago` in the Chrome flags for the nimble-components unit tests ## 🧪 Testing Build succeeds Verified locally that the `--time-zone-for-testing` Chrome flag is used as expected ## ✅ Checklist <!--- Review the list and put an x in the boxes that apply or ~~strike through~~ around items that don't (along with an explanation). --> - [ ] 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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request
🤨 Rationale
This PR adds a
placeholder
configuration to the following table columns as described in #1869:This is part of #1538. The remaining work is to add the
placeholder
attributes to the Angular and Blazor wrappers for each column.As part of this PR, I also included best practices established for each column in the storybook docs. These are adapted from the placeholder HLD.
👩💻 Implementation
I implemented this change in a similar way as #1914. Specifically, I updated
TableColumnTextCellViewBase
to have its own implementation ofcolumnConfigChanged
andcellRecordChanged
that calls a newapplyPlaceholderTextIfNeeded
function. This required me to create aTableColumnTextBaseCellRecord
interface to enforce that the cell records used byTableColumnTextCellViewBase
have avalue
property and to create aTableColumnTextBaseColumnConfig
interface to enforce that the column configs used byTableColumnTextCellViewBase
have an optionalplaceholder
property.🧪 Testing
✅ Checklist