preferred Popup anchor point can be customised for when anchor is dynamically anchoring

[anthony-hull]

A preference option for where the anchor is set when left to dynamically chose the best position.

Launch Checklist

• briefly describe the changes in this PR
• include before/after visuals or gifs if this PR includes visual changes
• write tests for all new functionality
• document any changes to public APIs
• post benchmark scores
• manually test the debug page
• tagged @mapbox/map-design-team @mapbox/static-apis if this PR includes style spec API or visual changes
• tagged @mapbox/gl-native if this PR includes shader changes or needs a native port
• apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
• add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog> Add anchorPreference option to PopupOptions which specifies the preferred anchor position when it is left to dynamically choose. </changelog>

[CLAassistant]

All committers have signed the CLA.

[SnailBones]

Thank you for the contribution @anthony-hull! I like this feature, and I've experienced this same pain point.

I'm not sure what behavior a developer should expect if they have anchor and anchorPreference options enabled with different directions. I think there's a more intuitive way to implement this: a boolean value ( maybe fixedAnchor as proposed in Issue #6965) that enables/disables adaptive anchor placement while placement always defaults to anchor.

To maintain behavior in existing applications, this boolean should default to false when anchor is undefined and true otherwise.

Your unit test is a great start! I'd like to see more thorough coverage of different possible states (combinations of options and positions on the map to ensure that the anchor is correctly dynamically anchoring).

Thanks again for your contribution! 🙏

Closes #6965

[diebarral]

@SnailBones I'm picking up on this, and I'm going to follow the fixedAnchor as a boolean approach.

However, I noticed you stated the following:

To maintain behavior in existing applications, this boolean should default to false when anchor is undefined and true otherwise.

If anchor is undefined, then I don't care about the value of fixedAnchor. If anchor is specificed, then there are two situations:

• If fixedAnchor is not specified, it should default to true to be backwards compatible.
• If fixedAnchor is specified, then it should be taken into account in the logic.

Would you agree?

[SnailBones]

@diebarral Agreed! To clarify, what's important is that the new feature maintains consistent behavior in existing apps (which is to have an adaptive anchor that defaults to "bottom" behavior when anchor === undefined.)

[anthony-hull]

Sorry I forgot about this! Thank you for offering to finish it.
I'd like to finish the feature if you don't mind?
I can get to this on Monday.

[diebarral]

@anthony-hull were you able to make some progress? If not, I have it done, I'm only missing some tests. Let me know if I can help you out with something.

[anthony-hull]

I'm just getting to it now :) my day job got busy!

[anthony-hull]

I have implemented the functionality. I will finish writing the tests tomorrow.

Mapbox.GL.JS.debug.page.mp4

[SnailBones]

Nit:

Suggested change

	(typeof (this.options.fixedAnchor) === 'undefined' && this.options.anchor)
	this.options.fixedAnchor === undefined && this.options.anchor)

[SnailBones]

Thanks for following this up @anthony-hull!

I tested this out and found one issue: popups with fixedAnchor: false and a non-bottom anchor don't correct expected when near the bottom of the screen, instead remaining in their default position.

I also added a few small nitpicks about code style.

Your added tests for default placement look great! Increasing test coverage for cases with fixedAnchor: false would be helpful for catching issues like this.

[SnailBones]

Could this be more readable if broken into two if statements?

[SnailBones]

Suggested change

	const fallbackPosition = 'bottom';
	const fallbackPosition = this.options.anchor || 'bottom';

I think this could help clean up the later return statements

[anthony-hull]

I'll get to this on Wednesday! thanks for the review.

[SnailBones]

@anthony-hull Apologies for the short notice, but we're planning a beta release tomorrow. I'd love to include this if you'd have the time to clean it up before then! If not, we can include it in the next release.

[anthony-hull]

I'll do it first thing tomorrow.
I live GMT so should be before you start work

[anthony-hull]

sorry tested positive for covid 🤕 I'll have to get to this later.

[SnailBones]

sorry tested positive for covid 🤕 I'll have to get to this later.

I'm sorry to hear that @anthony-hull! I hope you feel better soon.

[jovan-kecojevic]

@anthony-hull Hope you're feeling better by today, is there any progress on this one?