Skip to content

fix: fire config-update evnt on perspective-viewer#1728

Merged
texodus merged 1 commit intoperspective-dev:masterfrom
jkusa:column-menu-event
Mar 1, 2022
Merged

fix: fire config-update evnt on perspective-viewer#1728
texodus merged 1 commit intoperspective-dev:masterfrom
jkusa:column-menu-event

Conversation

@jkusa
Copy link
Contributor

@jkusa jkusa commented Mar 1, 2022

This PR resolves #1727 by firing the perspective-config-update event on perspective-viewer element.

This was tested manually. Is there a good place to add a test for this behavior?

@texodus texodus added the bug Concrete, reproducible bugs label Mar 1, 2022
@texodus texodus mentioned this pull request Mar 1, 2022
@texodus
Copy link
Member

texodus commented Mar 1, 2022

Thanks for the PR! Looks good!

I've added a documented integration test for this in #1730

@texodus texodus merged commit f50dc5d into perspective-dev:master Mar 1, 2022
@jkusa
Copy link
Contributor Author

jkusa commented Mar 1, 2022

Thanks @texodus!

I actually thought of a small enhancement to this fix:

const viewer = regularTable.closest("perspective-viewer");
viewer?.dispatchEvent(new Event("perspective-config-update"));

It is a little more specific on what it is trying to achieve and resilient to future DOM changes. I can push another PR or you can update it in #1730. Let me know what works best.

@jkusa jkusa deleted the column-menu-event branch March 1, 2022 21:25
@texodus
Copy link
Member

texodus commented Mar 1, 2022

I wouldn't worry about it unless it really bothers you 😄

The perspective source is riddled with rigid DOM structure lookups of this sort, sometimes for performance when a known structure is in a tight loop, but often just out of convenience/laziness/"readability". Personally, I prefer integration tests for behavior we want preserved, as opposed to scrutiny of the "robustness" of a particular implementation.

@jkusa
Copy link
Contributor Author

jkusa commented Mar 1, 2022

Sounds good :)

Personally, I prefer integration tests for behavior we want preserved, as opposed to scrutiny of the "robustness" of a particular implementation.

Yes 💯 Thanks for adding the test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Concrete, reproducible bugs

Development

Successfully merging this pull request may close these issues.

Column Styles Updates Do Not Fire perspective-config-update Event Correctly

2 participants