-
-
Notifications
You must be signed in to change notification settings - Fork 316
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
User Feedback UI widget #4270
Comments
IMO, a UI widget is overkill. I believe a public API to open the feedback form is enough. The user can be responsible for creating the button, link, or action that opens the form, as each app has its own visual identity. |
@brustolin, I think that's the main point of this new feature. If I'm not mistaken, people on the web love it, but I'm also unsure if folks on native mobile apps would use it. @bruno-garcia or @armcknight, maybe you can elaborate on why it's worth the effort to build a UI widget. |
@philipphofmann I believe the main point is the form screen isn't it? |
Yes, but I guess many iOS developers don't want a Sentry-flavored form screen but rather design their own. That was the main reason why we didn't add it in the past. |
There will be a way for customers to BYOB (bring your own button) and hook it into the integration. They would be completely responsible for displaying that button, we just use the touch event to display our form. I hadn't yet considered an option where we take a button from them, but manage displaying it. That could be another nice option, so they get their branding, but don't have to worry about the mechanics. In any case, it's not a requirement to use the widget. It's another option, there may be people out there that feel differently than you about this. It doesn't make sense to withhold the option from them just because someone here doesn't like it. Ideally we can attach analytics to it and we can see how many people use each method in practice. |
@armcknight, yes, that makes sense. Thanks for explaining 😀. |
Working to get the icon and text perfectly vertically centered in the button, but it's tough because different font faces have different amounts of space inside the text bounding box, so i need to find the right combination of constraints that work generally across them all. See for instance
|
I've got the font-lozenge centering complete, although it still results in slight variation in the top/bottom padding due to the differences in fonts' cap height and ascender values and how that affects how text is drawn inside a UILabel:
I think it's not worth pursuing equalizing that between fonts at this point, as people will pick a font they need, and then make the layout horizontal/vertical adjustments, and be done with it. And also scaling the megaphone with the size of the font:
I got stuck for a while on the Damascus font. It has some inverted measurements that make it very difficult to generalize the approach. After looking some more, I found out that that font is actually meant to draw Arabic characters, and when it renders Latin characters is when it gets wonky. Looks fine with Arabic:
I think it's fine to avoid dealing with that edge case. If someone complains, we should probably just tell them "don't do that" but we can always try to fix it at that point. |
If think it's OK to skip some edge cases in the first iteration. If people adopt the feature, use it, and complain, we can invest in fixing more edge cases. |
A couple other things I'll need to check:
|
Question: do we want the vertical padding to scale with the font size, or keep it fixed? With fixed spacing, it looks weirder the smaller the font gets, because the negative space gets larger. But with scaling, the larger the font, the more negative space. Maybe we scale it below the system default, and keep it fixed above?
|
Some right-to-left testing, to make sure the megaphone appears on the leading side, and faces the right way:
Guess there's still an issue with fonts that have inverted ascenders that will need to be addressed, judging from the arabic example (using Damascus font) Also, I would want the button on the other side of the screen. I previously had a FYI I found that some of the languages/locales to test this didn't work until I installed the following keyboards: Then I was able to test by changing the plist value |
We've decided to only offer this feature for iOS 13+ for now. If it becomes an issue we can always build in backwards compatibility. |
Description
Build the interface widget an end-user would tap to present the feedback capture form
The text was updated successfully, but these errors were encountered: