NIFI-15927 improvements to connector property group card#11237
NIFI-15927 improvements to connector property group card#11237scottyaslan wants to merge 4 commits into
Conversation
|
Reviewing... |
mcgilman
left a comment
mcgilman
left a comment
There was a problem hiding this comment.
Thanks for the PR @scottyaslan! Noted a few things below.
mcgilman
left a comment
mcgilman
left a comment
There was a problem hiding this comment.
Thanks for the updates @scottyaslan! Just noted one thing below.
mcgilman
left a comment
mcgilman
left a comment
There was a problem hiding this comment.
Thanks for the updates @scottyaslan! Left a few more comments.
| font-weight: 700; | ||
| } | ||
|
|
||
| .mat-mdc-card-subtitle { |
There was a problem hiding this comment.
We're removing global styles here. Jolt UI uses <mat-card-subtitle>. Have we verified these changes everywhere?
There was a problem hiding this comment.
oops forget those. I have restored the subtitle overrides. We didn't remove global styles we just moved them and updated the way we override them.
| const queryAll = (qa: string) => fixture.debugElement.queryAll(By.css(`[data-qa="${qa}"]`)); | ||
| const query = (qa: string) => fixture.debugElement.query(By.css(qa)); |
There was a problem hiding this comment.
Make both helpers handle the same kind of argument (e.g., both take a data-qa value, or both take a raw selector). Or maybe rename qa to selector in the query case.
| it('should show required asterisk for required properties', async () => { | ||
| const { query } = await setup(); | ||
| const content = query('mat-card-content'); | ||
| const asterisks = content.queryAll(By.css('.error-color')); |
There was a problem hiding this comment.
This will overmatch. There are multiple elements with this classname.
| title-text-size: 16px, | ||
| title-text-line-height: 20px, | ||
| title-text-weight: 600, |
There was a problem hiding this comment.
My comment previously was that in that instance the card title was too big. The change would affect all card titles throughout all applications. Is this appropriate?
There was a problem hiding this comment.
I think this is ok. We mainly just reduced the title by 2px and adjusted its line height to 20px. The change I made here is to correctly leverage the angular material mixins to do so. Here is the jolt UI wilth these changes:
2 participants
Summary
NIFI-15927
Tracking
Please complete the following tracking steps prior to pull request creation.
Issue Tracking
Pull Request Tracking
NIFI-00000NIFI-00000VerifiedstatusPull Request Formatting
mainbranchVerification
Please indicate the verification steps performed prior to pull request creation.
Build
./mvnw clean install -P contrib-checkLicensing
LICENSEandNOTICEfilesDocumentation