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

chore(widgets) cleanup widget constructors #9312

Merged
merged 3 commits into from
Dec 22, 2024

Conversation

chrisgervang
Copy link
Collaborator

For #9056

Background

Pulling non-react changes out of #9278

Change List

  • copy props instead of mutating
  • use nullish coalescing operator in constructor
  • fix html title in compass widget

@coveralls
Copy link

coveralls commented Dec 20, 2024

Coverage Status

coverage: 91.703%. remained the same
when pulling f2c55ef on chr/cleanup-widget-constructors
into 17ab465 on master.

@chrisgervang chrisgervang mentioned this pull request Dec 21, 2024
36 tasks
props.label = props.label || 'Compass';
props.style = props.style || {};
this.props = props;
this.id = props.id ?? this.id;
Copy link
Collaborator

Choose a reason for hiding this comment

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

How much do we gain by copying these over to field rather than just accessing them from this.props? Seems like a bunch of boilerplate code for widgets that might not be necessary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe one line of boilerplate would be saved since id is the only required member.

The projected widgets like Tooltip or Marker don't expose the placement member as a props since they're always in "fill".

I see the duplicate code. Maybe an abstract class could help?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think the case for an abstract class is quite black and white, at least not until the amount of shared code grows further.
I agree that having all code visible and not relying on base class magic is also valuable.

@chrisgervang chrisgervang merged commit 7a23912 into master Dec 22, 2024
4 checks passed
@chrisgervang chrisgervang deleted the chr/cleanup-widget-constructors branch December 23, 2024 06:26
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.

3 participants