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

Fix respecting font bounds #573

Merged
merged 7 commits into from
Mar 26, 2021
Merged

Conversation

kuba2k2
Copy link
Contributor

@kuba2k2 kuba2k2 commented Mar 23, 2021

Yesterday, I have failed multiple times trying to evenly position an icon within a circle, just to find out there is an option to respectFontBounds, which is disabled by default. Enabling that manually on each of dozens IconicsDrawables in my app would be a nightmare, which even turns to impossible for views added in XML (no such attribute).
This PR adds a global, default option (in the Iconics singleton) to enable respectFontBounds on every drawable/XML view.

Apparently, while the option was present, it had some issues. Sure, it did not scale the icons up (such as gmd_arrow_drop_down), but somehow the padding was still cropped out of all icons having uneven left-right paddings.
Pictures
(the right part comes from the Layout Inspector)

The option in the PR fixes this behavior (to truly fit the whole glyph in the canvas) and allows the user to add icon padding while still preserving the font's built-in padding.

The sample activity is updated to include a checkbox to toggle this option (having effect only in the large popup). The icon in each item has this always enabled, let me know if I should also change this.

@mikepenz
Copy link
Owner

thank you so much @kuba2k2 for this PR

@kuba2k2
Copy link
Contributor Author

kuba2k2 commented Mar 25, 2021

Hey, does this PR require any further changes, or would it be ok for you to merge/release it?

@mikepenz
Copy link
Owner

@kuba2k2 the PR itself should be fine. I haven't had time to review it in depth yet, which I need to do before merge, want to ensure no behaviour change for existing users

@mikepenz
Copy link
Owner

Thank you so much. I'll include this in a beta first to see if the adjustment have any un-anticipated side effects

@mikepenz mikepenz merged commit a4c2260 into mikepenz:develop Mar 26, 2021
@kuba2k2
Copy link
Contributor Author

kuba2k2 commented Mar 26, 2021

Thanks, I can now start adapting my app to usage with respectFontBoundsDefault as true. As you probably already know, with this option set, all icons would be smaller - because of the font's inbuilt padding. Although I did not notice any changes between 5.2.8 and my PR (when leaving the option as default - false).

@mikepenz
Copy link
Owner

mikepenz commented Mar 26, 2021

@kuba2k2 thank you for opening the PR :)

Yes indeed. I checked and compared. things looked good, just wanting to play it double safe to not break somebodies integration un-intentional. having it in beta for a bit will hopefully give people enough time to pre-test and report if they see something strange.

After all the code changed a bit on sizing and positioning the font

@kuba2k2
Copy link
Contributor Author

kuba2k2 commented Mar 26, 2021

Hey, will/is the 5.3.0-b01 published to Maven Central? I can't find nor resolve it in Gradle, are you waiting for some kind of a review or acceptance (never published to Maven Central) or haven't uploaded it yet? Not that I'm hurrying you up, just want to know if there's something wrong on my side.

@mikepenz
Copy link
Owner

@kuba2k2 thanks so much for pinging. totally forgot to do the staging procedure. so after publishing the staging repo needs to be closed and then released.

after publishing to bintray (where this is not required) for a few years I totally forgot.

Did this now. should be up in 30-60 min.

all should be good on your end

@kuba2k2
Copy link
Contributor Author

kuba2k2 commented Mar 26, 2021

Huh, now I see this change may actually be problematic (if the default value gets enabled). For example, consider the MaterialDrawer's primary item, which sets a 1dp padding to a 24dp icon, leaving 22dp for the glyph, which is now only 18dp for the icon itself. As per the Material Design guidelines, the icon should be 24dp for the glyph. Do you know why is this 1dp padding added in MaterialDrawer? Is it maybe because the paddings were cropped by Iconics at the time? Should we maybe update MD to respect font bounds and remove the 1dp padding? I'm asking, because the icons now look so... tiny..

@mikepenz
Copy link
Owner

The 1dp padding was added back in the days to ensure the right size of the icons to match the exact spec. Especially with fonts which do not have the proper paddings this was super important.

The community material icon font luckily includes them precisely.

I wonder though what the right decision here is. ultimately it may break projects silently (by the paddings being wrong)

would for your usecase maybe an extended IconicsDrawable or extension function work which will automatically enable the respect of the bounds wherever you use them? (so you would not need the global flag?)

@kuba2k2
Copy link
Contributor Author

kuba2k2 commented Mar 27, 2021

Not really, I think, because I'm using the IconicsImageView in some places, and even then I want to respect the bounds in most cases. I will probably update the MD's ImageHolder to just ignore the 1dp padding, as I'm using a custom holder anyway.

As for the 1dp padding maybe you're right it should remain there. Or maybe there should also be an option to enable/disable it, according to which font the developer uses. So if the app uses for example FontAwesome (in which the paddings are totally broken, btw) the developer would enable the inbuilt padding, but I, using CMD, would disable it easily.

@kuba2k2 kuba2k2 deleted the feature/respect-bounds branch October 10, 2021 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants