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

Simplify props API of ExportMenu #1237

Merged
merged 1 commit into from
Oct 20, 2022
Merged

Simplify props API of ExportMenu #1237

merged 1 commit into from
Oct 20, 2022

Conversation

axelboc
Copy link
Contributor

@axelboc axelboc commented Oct 20, 2022

Preparation for #1063

The getFormatURL prop was confusing. It made it look like the URLs were generated lazily, which isn't the case -- they're generated during render. Also, we used to end up with two arrays, formats and urls, so we had to access one by index in the map loop.

Copy link
Member

@loichuder loichuder left a comment

Choose a reason for hiding this comment

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

Nice

You could filter exportEntries to remove entries with url = undefined before passing it to ExportMenu. This would remove some checks in ExportMenu and avoid passing entries that are not relevant to the ExportMenu.

@axelboc
Copy link
Contributor Author

axelboc commented Oct 20, 2022

I had this at first, but it meant that the toolbars had to do the filtering, which felt like a bit too much unnecessarily duplication. It may evolve again, so I went for what felt like the simplest solution first.

@loichuder
Copy link
Member

I had this at first, but it meant that the toolbars had to do the filtering, which felt like a bit too much unnecessarily duplication. It may evolve again, so I went for what felt like the simplest solution first.

Sure, no qualms about merging this as it is

@axelboc axelboc merged commit edcb0f7 into main Oct 20, 2022
@axelboc axelboc deleted the refact-export-menu branch October 20, 2022 15:16
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.

None yet

2 participants