Skip to content

fix(hierarchical grid): add richer data and a few new columns#3258

Merged
hanastasov merged 5 commits intovNextfrom
EWhite/3250-grid-samples-height
Feb 23, 2023
Merged

fix(hierarchical grid): add richer data and a few new columns#3258
hanastasov merged 5 commits intovNextfrom
EWhite/3250-grid-samples-height

Conversation

@multignite
Copy link
Copy Markdown
Contributor

Closes #3250

export class HGridToolbarPinningComponent implements OnInit {
public localdata;
public dark = false;
public dark = "";
Copy link
Copy Markdown
Contributor

@hanastasov hanastasov Feb 23, 2023

Choose a reason for hiding this comment

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

I see why it is done this way (setting it as string) and it works out nicely when you receive a string "true" from the query params. But imagine if one loads the url with the following params: "?dark=justSomeRandomString". You end up with unexpected values in your property.

In general, IMO, it is good to keep the value boolean, and the check whether it is a "true" string being moved from the template to the place where you set the value on line 21:

this.dark = params.dark === 'true' ? true : false;

If using this ternary operator seems verbose, depending on the case a Null coalescing operator or a Logical OR operator can be used.

Copy link
Copy Markdown
Contributor

@hanastasov hanastasov Feb 23, 2023

Choose a reason for hiding this comment

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

Null coalescing operator serves a more specific purpose, so when I think about it the Logical OR operator would serve us best here.

OMG, i'm over complicating things :D we can go with only the === operator :D

Comment thread src/app/data/hierarchical-data.ts
@@ -1,4 +1,4 @@
<div [ngClass]="{'grid__wrapper': true, 'dark-theme': dark === true }" style='width: 100%;'>
<div [ngClass]="{'grid__wrapper': true, 'dark-theme': dark === 'true' }" style='width: 100%;'>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

After keeping the dark value boolean, you will be able to use a simpler expression here:

<div [ngClass]="{'grid__wrapper': true, 'dark-theme': dark }" style='width: 100%;'>

Then it seems even nicer to rename the constant to smth more descrptive, for example useDarkTheme, and then the template seems even more readable:

<div [ngClass]="{'grid__wrapper': true, 'dark-theme': useDarkTheme }" style='width: 100%;'>

@hanastasov hanastasov merged commit 6c76ff2 into vNext Feb 23, 2023
@hanastasov hanastasov deleted the EWhite/3250-grid-samples-height branch February 23, 2023 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants