-
Notifications
You must be signed in to change notification settings - Fork 540
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
[JS] Table element #5705
[JS] Table element #5705
Conversation
- ac-typed-schema@0.6.0 - aclint@0.2.0 - adaptivecards-controls@0.7.0 - adaptivecards-designer-app@0.5.2 - adaptivecards-designer@2.1.0 - adaptivecards-extras-testapp@0.1.1 - adaptivecards-extras@0.1.1 - adaptivecards-fabric@1.1.2 - adaptivecards-site@0.7.0 - adaptivecards-templating@2.1.0 - adaptivecards-visualizer@1.3.2 - adaptivecards@2.8.0 - marked-schema@0.1.7 - spec-generator@0.6.1
Hi @dclaux. This non-spec pull request has had no recent activity for the past 5 days . Please take the necessary actions (review, address feedback or commit if reviewed already) to move this along. |
Hi @dclaux; Thanks for taking action on your previously stale pull request. Resetting staleness. |
Hi @dclaux. This non-spec pull request has had no recent activity for the past 5 days . Please take the necessary actions (review, address feedback or commit if reviewed already) to move this along. |
Hi @dclaux; Thanks for taking action on your previously stale pull request. Resetting staleness. |
source/nodejs/adaptivecards-extras-designer/src/adaptivecards-extras-designer.ts
Show resolved
Hide resolved
import { Strings } from "./strings"; | ||
import { stringToCssColor } from "./utils"; | ||
|
||
export class ColumnDefinition extends SerializableObject { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel like this should be TableColumnDefinition
to prevent confusion with ColumnSet
Column
s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really mind, but this is per spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you feel strongly about this one, please send a mail to the team to suggest this name change which would also have to be reflected in the spec that Risheek has a PR for here: #5806
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to hold this PR on the conversation, so approving :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all the hard work here David, looks good!
Not blocking this time, but in the future could you please break up large changes into smaller packages to review? Large code drops are difficult to properly review, so it'll help get prompter and better feedback in the future. Thank you!
"adaptivecards": "^2.9.0", | ||
"adaptivecards-controls": "^0.8.0", | ||
"adaptivecards-designer": "^2.2.0", | ||
"adaptivecards": "^2.8.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we're downgrading the Adaptive Cards packages? Or am I reading that incorrectly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. Typically, we should NOT up this dependency version every time we release a new adaptivecards package, unless that package introduces new APIs that are required by the designer. 2.8.0 is a good minimal version to stick to for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And... let me contradict myself. The designer has dependencies on the breaking changes we are making to support text styles, so this dependency needs to be pointing to the latest version this time around. I'll update.
* Publish - ac-typed-schema@0.6.0 - aclint@0.2.0 - adaptivecards-controls@0.7.0 - adaptivecards-designer-app@0.5.2 - adaptivecards-designer@2.1.0 - adaptivecards-extras-testapp@0.1.1 - adaptivecards-extras@0.1.1 - adaptivecards-fabric@1.1.2 - adaptivecards-site@0.7.0 - adaptivecards-templating@2.1.0 - adaptivecards-visualizer@1.3.2 - adaptivecards@2.8.0 - marked-schema@0.1.7 - spec-generator@0.6.1 * Initial commit * Working through package hell * End of package hell? * Table - WIP * Table - WIP * Table - WIP * Table - still WIP * Table - almost done * Small bug fix * Fix newly dropped card element initialization * Tweaks * Move to flex, <table> is too constraining * Col header txt style, cascading h and v alignments * Adjust cellSpacing to 8 (4 isn't enough) * Move Table to main package * Add column editing support in designer * Fix treeview ordering issue * Add missing copyright headers * Explicit color variable names in ProgressBar * Rearrange code to fix build break * TableColumnDefinition + pkg version updates
Related Issue
Description
Microsoft Reviewers: Open in CodeFlow