Skip to content

Feature template picker #993

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

Open
wants to merge 32 commits into
base: v.next
Choose a base branch
from
Open

Feature template picker #993

wants to merge 32 commits into from

Conversation

rolson
Copy link
Contributor

@rolson rolson commented Dec 28, 2024

We have one of these in the objc toolkit.

This is an initial PR. I still need to add tests and a tutorial. Also the map that I use in the example is not a good map for this. I need to find a better map.

image

@rolson
Copy link
Contributor Author

rolson commented Dec 31, 2024

I added tests

@rolson rolson requested a review from dfeinzimer December 31, 2024 18:13
@rolson
Copy link
Contributor Author

rolson commented Dec 31, 2024

ok, added tutorial now as well

Copy link
Member

@mhdostal mhdostal left a comment

Choose a reason for hiding this comment

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

I've tested this with the Feature Forms Micro App. It looks and works great! I noticed a couple of issues, which I've commented on. I'll look at the code in another go-round.

@rolson
Copy link
Contributor Author

rolson commented Jan 2, 2025

Actually, I'm going to look for a different solution because I'm not sure I like how it resizes some of these:

With the fix:
image

Before the fix:
image

@rolson
Copy link
Contributor Author

rolson commented Jan 2, 2025

Ok, I fixed the image size so that it will limit it to 50x50, but not enlarge the smaller icon sizes.

@mhdostal
Copy link
Member

mhdostal commented Jan 3, 2025

Ok, I fixed the image size so that it will limit it to 50x50, but not enlarge the smaller icon sizes.

The only issue I see with that is when Dynamic Type is used the text will increase in size but not the icons. That would be "nice-to-have", but it's not a dealbreaker for me.

image

I did add padding around the image to keep the size in check.

Image(uiImage: image)
    .resizable()
    .aspectRatio(contentMode: .fit)
    .padding(4)

Here's what it looks like with Dynamic Type:

image

And without it:

image

@rolson
Copy link
Contributor Author

rolson commented Jan 3, 2025

The only issue I see with that is when Dynamic Type is used the text will increase in size but not the icons. That would be "nice-to-have", but it's not a dealbreaker for me.

I'd rather have features that are under 50x50 draw correctly in their correct size. To me that's important. If there is a way to do that and let the larger features grow up to the dynamic font size, that would be ideal. But I couldn't find a way to do that unfortunately.

@philium
Copy link
Contributor

philium commented Jan 3, 2025

The tool for that job is ScaledMetric.

@rolson rolson force-pushed the ryan/featureTemplatePicker branch from 32af9db to 3ffb37d Compare January 3, 2025 20:46
@rolson rolson requested review from mhdostal and dfeinzimer January 3, 2025 21:58
Copy link
Collaborator

@dfeinzimer dfeinzimer left a comment

Choose a reason for hiding this comment

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

Just 2 last comments and I think that'll be it.

Comment on lines +262 to +263
)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should the templates be sorted alphabetically by name?

Suggested change
)
}
)
infos.sort { $0.template.name < $1.template.name }
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps the sections too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure. I could see alphabetizing them. Currently it is in the order of layers and then the templates that the layers report. So I'm not sure what is desirable. @mhdostal do you recall what the obj-c based version did?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure. I could see alphabetizing them. Currently it is in the order of layers and then the templates that the layers report. So I'm not sure what is desirable. @mhdostal do you recall what the obj-c based version did?

There was no sorting in the obj-c template picker.

@mhdostal
Copy link
Member

mhdostal commented Jan 6, 2025

One enhancement request would be to only show the templates that apply to a specific geometry type. In the Swft FF test app, we are only allowing creating point symbols, so limiting the feature templates to those supporting point geometries would be nice.

mhdostal
mhdostal previously approved these changes Jan 6, 2025
@rolson
Copy link
Contributor Author

rolson commented Jan 7, 2025

One enhancement request would be to only show the templates that apply to a specific geometry type. In the Swft FF test app, we are only allowing creating point symbols, so limiting the feature templates to those supporting point geometries would be nice.

Cool. Did the original template picker in the other toolkit do this as well?

@mhdostal
Copy link
Member

mhdostal commented Jan 7, 2025

One enhancement request would be to only show the templates that apply to a specific geometry type. In the Swft FF test app, we are only allowing creating point symbols, so limiting the feature templates to those supporting point geometries would be nice.

Cool. Did the original template picker in the other toolkit do this as well?

No, but it's something that I'm doing for the Swift Feature Form test app; we're not concerned with the full geometry-editing experience yet, so we're only supporting adding point features.

dfeinzimer
dfeinzimer previously approved these changes Jan 7, 2025
Copy link
Collaborator

@dfeinzimer dfeinzimer left a comment

Choose a reason for hiding this comment

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

Updated the image. PR looks good 👍

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 participants