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

Fix group and subgroup expansion #1552

Merged
merged 2 commits into from
Aug 3, 2022
Merged

Conversation

sweco-sedalh
Copy link
Contributor

@sweco-sedalh sweco-sedalh commented Jun 7, 2022

This PR reverts the component refactor from #1529, fixing group and subgroup expansion.

The refactor of the base component in #1529 slightly changed how events where attached, which made it possible for onRender to be called more than once which in turn attached event handlers more than once, resulting in bugs like in #1551.

This PR fixes that by somewhat formalizing (and reducing) the handling of especially the "render" event. There is also commented out code to get a warning on similar issues (currently present in the print component, though without any user visible issues that I could find).

Closes: #1551

@tonnyandersson
Copy link
Collaborator

This PR may potentially have a negative impact on evertything that uses ui components (controls, plugins, whatevers). We have an upcoming release and we do not find the idea of potentially introducing a myriad of bugs in that release particularly appealing.

The changes made to the base component in #1529 will probably be undone very soon, which will make this PR obsolete. The changes were not necessarily bad, but they should have been made in a separate PR and should not have been merged this close to a release because of the broad impact such fundamental changes might have.

@sweco-sedalh
Copy link
Contributor Author

To bad that no one highlighted this during the 5(!) weeks that the original PR was up for review... I even put the refactor into a separate commit there to make it easy to remove from that PR in case there would have been objections.

Changed this PR to revert the previous commit and created a new PR (#1553) for the component refactor. I hope that one will not be laying around another 5 weeks so that the time can instead be used to fix this "myriad" of bugs your speaking off.

@tonnyandersson
Copy link
Collaborator

tonnyandersson commented Jun 9, 2022

You're absolutely right. I think it was overlooked because development and testing/auditing was done within the scope of the actual issue.

@steff-o
Copy link
Contributor

steff-o commented Jun 16, 2022

@sweco-sedalh Could you also revert the changes that introduced the lint errors?

@sweco-sedalh
Copy link
Contributor Author

@steff-o I fixed them rather than reverting anything further. Lets move forward rather than backward!

I also fixed some preexisting lint issues in viewer.js, fixed the definition of the lint command (the reason I hadn't seen the issues before was because the command was broken) and added a Github Actions workflow to lint everything which should help to catch this in the future.

I recommend not squashing this PR on merge since the two commits are quite distinct.

@steff-o
Copy link
Contributor

steff-o commented Jun 17, 2022

@sweco-sedalh Are you running linux? The lint script works on my machine (windows). I recently changed it in #1524 as previously it did not recurse the src directory and origo.js was omitted. I changed it to something that I tested to work on windows. Your approach also seems to work.

However, I strongly oppose to adding the lint Github workflow in this PR. I only asked to get the lint errors that were introduced fixed somehow (reverted or rewritten or whatever). The workflow is a great addition, but according to our developing guideline it should be a separate issue and PR.

@sweco-sedalh
Copy link
Contributor Author

@sweco-sedalh Indeed (WSL to be correct). Recursive glob (**) is not enabled by default in bash and thus won't work for most.

I have split out the workflow and lint issues not introduced by #1529 into #1561.

@tonnyandersson
Copy link
Collaborator

I guess this one is good to go. @jokd, merge it if your approval still stands.

@jokd jokd merged commit b078c1f into origo-map:master Aug 3, 2022
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.

Legend groups and subgroups broken
4 participants