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.

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 support to clear empty grid label #105

Merged
merged 1 commit into from Jan 15, 2024
Merged

Add support to clear empty grid label #105

merged 1 commit into from Jan 15, 2024

Conversation

flang
Copy link
Contributor

@flang flang commented Dec 11, 2023

Close #103

Copy link

sonarcloud bot commented Dec 11, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@javier-godoy
Copy link
Member

clearEmptyGridLabel keeps the component attached to the DOM but removes the association with the helper (it's a "component leak"), and now, I'm confused about why 8ae45df was needed. The intent was that setEmptyGridLabel(null) would remove the label.

@flang
Copy link
Contributor Author

flang commented Dec 14, 2023

Removing the empty grid label from its parent shouldn't be helper's responsibility IMHO. Helper should only make use of it.

@javier-godoy
Copy link
Member

Removing the empty grid label from its parent shouldn't be helper's responsibility IMHO. Helper should only make use of it.

Good point. Helper does not add the label, hence removing it is not Helper's responsibility. However, there is a behavior change because calling setEmptyGridLabe twice will keep both components visible, but only the last label will synchronize with the provider..

Shouldn't it be implemented as follows? What do you think?

-    emptyLabel = component;
-    removeRegistration();
+   clearEmptyGridLabel();
+   emptyLabel = component;

I think there is an implicit contract that, whenever I call set, the old thing that has been set is now unset (whatever that means).

Copy link

sonarcloud bot commented Jan 15, 2024

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@flang
Copy link
Contributor Author

flang commented Jan 15, 2024

Let's just hide any existing empty grid label whenever set is call

@javier-godoy javier-godoy merged commit 193a0dc into master Jan 15, 2024
4 checks passed
@javier-godoy javier-godoy deleted the issue-103 branch January 15, 2024 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide a way to clear empty grid label
2 participants