-
Notifications
You must be signed in to change notification settings - Fork 364
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 basemaps order #7170
base: main
Are you sure you want to change the base?
Fix basemaps order #7170
Conversation
) { | ||
const itemModel = this.terria.getModelById(BaseModel, baseMapItem.item); | ||
if (MappableMixin.isMixedInto(itemModel)) | ||
this.enabledBaseMaps.forEach((baseMapItem) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.enabledBaseMaps
might be undefined, and when undefined it should show all base maps available.
i.e. simple sorting before return in the old code would do the trick
// Sort based on enabledBaseMaps order
if (this.enabledBaseMaps) {
enabledBaseMaps.sort((a, b) => {
const aIndex = this.enabledBaseMaps!.indexOf(a.item.uniqueId!);
const bIndex = this.enabledBaseMaps!.indexOf(b.item.uniqueId!);
return aIndex - bIndex;
});
}
another option that comes to my mind is to go with the utility function and do mapping over proper array
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zoran995 what do you think about
const baseMapList =
this.enabledBaseMaps ?? this.items.map((item) => item.item);
baseMapList.forEach((baseMapItem) => {
const item = this.items.find((item) => item.item === baseMapItem);
if (item && !ModelReference.isRemoved(baseMapItem)) {
const itemModel = this.terria.getModelById(BaseModel, baseMapItem);
if (MappableMixin.isMixedInto(itemModel)) {
enabledBaseMaps.push({
image: item.image,
contrastColor: item.contrastColor,
item: itemModel
});
}
}
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on first look, it should work correctly and the rest is a personal preference on the code structure. It would be great to cover sorting with the unit test to avoid regression bugs in future, there are tests for BaseMapsModel
What this PR does
Fixes #7169
Checklist
doc/
.