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

Rename MenuDropdown component to MenuItemsDropdown to avoid any case mismatches #19156

Merged
merged 2 commits into from May 2, 2022

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented Apr 28, 2022

Description:

Even though most people might have already updated and solved the filename case issue, there could be still people running into that issue if they had installed a version before the rename and update to a newer version.

Simply renaming the component should finally solve that and consume less time than trying to dig longer in the code where it might fail to remove or recreate a file on update when the name only differs in case. We for sure should keep that in mind and should avoid renaming files in case only.

Note: The angularjs adapter still uses the old name as directive, so it can't break any plugins that might still use the old angular js directive. Which is e.g. also the case here:

fixes #18724

Review

@sgiehl sgiehl added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels Apr 28, 2022
@sgiehl sgiehl added this to the 4.10.0 milestone Apr 28, 2022
@diosmosis
Copy link
Member

What was the other file that MenuDropdown was in conflict with? Was it in the same folder? And this is a problem during the update?

@diosmosis
Copy link
Member

Ok I saw the related issue, makes sense.

@@ -92,7 +92,7 @@ export { default as SideNav } from './SideNav/SideNav';
export { default as EnrichedHeadline } from './EnrichedHeadline/EnrichedHeadline.vue';
export { default as ContentBlock } from './ContentBlock/ContentBlock.vue';
export { default as Comparisons } from './Comparisons/Comparisons.vue';
export { default as MenuDropdown } from './MenuDropdown/MenuDropdown.vue';
Copy link
Member

Choose a reason for hiding this comment

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

Technically this is a bit of a BC break if a plugin uses MenuDropdown. It could potentially also be exported as MenuDropdown and MenuItemsDropdown.

No plugin probably uses it, though, so it's probably fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

@diosmosis but a plugin might need to use the vue component, right? The Angular JS component should still work as before.
I've looked through all our plugins and it wasn't used. I doubt any third party plugin is already migrated to vue...

Copy link
Member

Choose a reason for hiding this comment

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

Yes a plugin might need to use it which is why changing the export name (MenuDropdown -> MenuItemsDropdown) can be seen as a BC break. It's not important here but worth keeping in mind for the future I think after Matomo 5 when plugins may use them directly.

@justinvelluppillai justinvelluppillai merged commit 6375fa5 into 4.x-dev May 2, 2022
@justinvelluppillai justinvelluppillai deleted the rename_menudropdown branch May 2, 2022 05:40
@justinvelluppillai justinvelluppillai removed not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels May 8, 2022
@justinvelluppillai justinvelluppillai changed the title Rename MenuDropdown component to MenuItemsDropdown to finally avoid any case mismatches Rename MenuDropdown component to MenuItemsDropdown to avoid any case mismatches May 8, 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.

4.7.0 After triggering the automatic update a fatal error occurres
3 participants