jlnrrg / simple_icons

The Simple Icon pack available as Flutter Icons. Provides over 1500 Free SVG icons for popular brands.
Creative Commons Zero v1.0 Universal
25 stars 6 forks source link

Icons not truly centered on web #11

Open Trulsmatias opened 3 years ago

Trulsmatias commented 3 years ago

As with you I used https://github.com/NikSWE/flutter_brand_icons but ran in to the issue with null safety. Fortunately I came over your comment on an issue on that repo that led me to this repo https://github.com/NikSWE/flutter_brand_icons/issues/1#issuecomment-772851138. So I know you took inspiration from that repo. Unfortunately this repo is not centering the icons as the other repo did. This is not a breaking issue, but annoying.

flutter_brand_icons:

Skjermbilde 2021-04-04 kl  15 09 40

this repo:

Skjermbilde 2021-04-04 kl  15 10 11
jlnrrg commented 3 years ago

Both libraries just export IconData, so I highly doubt that this is an issue with the package. If you'd like to provide an minimal example of the error you encounter, I will be more than happy to take a look :smile:

Trulsmatias commented 3 years ago

Thanks for the fast reply. I have had a lot on my plate lately and haven't had the chance to look into this before now. After doing some testing it looks like this only is a problem on web (which is where I am using the package). Here is a minimal example:

import 'package:flutter/material.dart';
import 'package:flutter_brand_icons/flutter_brand_icons.dart';
import 'package:simple_icons/simple_icons.dart';

void main() {
  runApp(MyApp());
}

class MyApp extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      home: Scaffold(
        appBar: AppBar(
          title: Text("Icon Demo"),
        ),
        body: Column(
          mainAxisAlignment: MainAxisAlignment.center,
          children: [
            Row(
              mainAxisAlignment: MainAxisAlignment.center,
              children: [
                InkedIconButton(
                  icon: BrandIcons.apple,
                ),
                SizedBox(
                  width: 1,
                ),
                InkedIconButton(
                  icon: SimpleIcons.apple,
                ),
              ],
            ),
            SizedBox(
              height: 10,
            ),
            Row(
              mainAxisAlignment: MainAxisAlignment.center,
              children: [
                InkedIconButton(
                  icon: BrandIcons.googleplay,
                ),
                SizedBox(
                  width: 1,
                ),
                InkedIconButton(
                  icon: SimpleIcons.googleplay,
                ),
              ],
            )
          ],
        ),
      ),
    );
  }
}

class InkedIconButton extends StatelessWidget {
  final IconData icon;

  const InkedIconButton({Key key, this.icon}) : super(key: key);

  @override
  Widget build(BuildContext context) {
    return Ink(
      decoration: ShapeDecoration(
        color: Colors.black,
        shape: RoundedRectangleBorder(
            borderRadius: BorderRadius.all(Radius.circular(10))),
      ),
      child: IconButton(
        iconSize: 35,
        icon: Icon(
          icon,
          color: Colors.white,
        ),
        onPressed: () {},
      ),
    );
  }
}
jlnrrg commented 3 years ago

Thanks for limiting the issue to flutter web. Because the library just exports IconData and it does not differenciate between web or app, the only explanation I can think if is that there is an issue directly with the flutter web framework. But I agree, that is is weird that such a problem did not occur with the brand_icon. Maybe @NikSWE (author of brand_icons) has an idea.

Trulsmatias commented 3 years ago

Let's hope he answers :)

jmarkham commented 3 years ago

It's the font file. Had the same issue with flutter web and html renderer. Issue doesn't appear with canvaskit. To validate it's something with the ttf file, I can download the SVGs I want from simpleicons.org, upload them to fluttericon.com, download the result, and use them as-is with the rendering as expected in both html and canvaskit rendering.

NikSWE commented 3 years ago

It's the font file

I agree with @jmarkham's explanation, also @jlnrrg how are you converting the svg from simpleicons to ttf file? (I've used icomoon to convert the svg's set to ttf file, manual process)

NikSWE commented 3 years ago

@Trulsmatias apart from the two icons mentioned which other icons are showing such difference ?

NikSWE commented 3 years ago

if we could use svg instead of ttf on flutter web that would be great, would suggest @jlnrrg to ask this on the flutter repo for better guidance.

jlnrrg commented 3 years ago

@NikSWE I only grab the ttf file from simpleicons directly (I have no idea how they build it) with the ttf file I run ttx -t cmap SimpleIcons.ttf to get ttx file where I can grab the code from. (it is basicaly xml)

NikSWE commented 3 years ago

@jlnrrg when I started my project they didn't even have a system to build the ttf file (if i remember correctly they were only building woff and woff2 font files)

NikSWE commented 3 years ago

can't we create a sample flutter project for web that showcases all the icons? something like this

NikSWE commented 3 years ago

this way people can quickly get the name of the icon (specified in dart code)

jlnrrg commented 3 years ago

I think they added it later github issue

I don't really see the benefit of it in regards to the effort, as people can just use that page or look in the documentation. But if you want to build it, feel free to do so, and make a PR or publish it in a seperate repo, where we can link to then.

jlnrrg commented 3 years ago

So based on their build script everything looks good to my amateur eyes. Also they did not notice any error concerning the positioning.

apart from the ttffile I convert rn, they also provide eot, otf, woff, woff2. As I feel reluctant to try a lot of convert methods, let me first ask if anyone has any experience in this field.

If that is the case I am more than willing to open a beta branch, so you can try if your issue is solved.

To remind, this is what I do rn:

  1. ttx -t cmap SimpleIcons.ttf -> generates SimpleIcons.ttx
  2. import the ttf file in pubspec.yaml
  3. use IconData to access the codepoint out of the generated .ttf (which I know from SimpleIcons.ttx)
NikSWE commented 3 years ago

does this issue only happen in html renderer on web ?

NikSWE commented 3 years ago

did someone try this on any flutter desktop build ?

NikSWE commented 3 years ago

@jlnrrg you can look at this repo you might figure out how did they go about this issue (they also created a new icon class for positioning)

NikSWE commented 3 years ago

also pls try the above mentioned package and test it on web (html renderer)

Trulsmatias commented 3 years ago

@Trulsmatias apart from the two icons mentioned which other icons are showing such difference ?

I only use those two. Have not checked other icons.

jlnrrg commented 2 years ago

@Trulsmatias @jmarkham in the latest version 6.1.0 the issue should have been fixed. Can you please confirm, that the error is also resolved on your side?

Trulsmatias commented 2 years ago

This is how it looks like now. Red is SimpleIcons, blue is BrandIcons.

Versions in pubspec.yaml

  simple_icons: ^6.1.0
  flutter_brand_icons: 1.1.0

web - canvaskit. Blue looks correct as it gets all the way to the top edge

web - html. Even worse unfortunately.

web - html - centered to better visualize the problem. Red goes outside the screen.

Double-checked on mobile and the problem was there too, so maybe the scope of this issue should be reverted back to including mobile again:

Android emulator. Blue gets all the way to the top, red doesn't.

iPhone simulator. This was interesting, on all the other images blue was higher than red. On iPhone this was reversed, so red overshoots outside the screen. Again blue reaches the edge as expected.

Code I used:

import 'package:flutter_brand_icons/flutter_brand_icons.dart';
import 'package:simple_icons/simple_icons.dart';

void main() {
  runApp(const MyApp());
}

class MyApp extends StatelessWidget {
  const MyApp({Key? key}) : super(key: key);

  @override
  Widget build(BuildContext context) {
    const double iconSize = 400;
    return MaterialApp(
      home: Scaffold(
        body: Stack(
          children: [
            Icon(
              SimpleIcons.apple,
              color: Colors.red,
              size: iconSize,
            ),
            Icon(
              BrandIcons.apple,
              color: Colors.blue,
              size: iconSize,
            ),
          ],
        ),
      ),
    );
  }
}
jlnrrg commented 2 years ago

@Trulsmatias thank you for your feedback. I was able to reproduce the issue.

@NikSWE I tried your flutter_brand_icons package with the most recent SimpleIcons.ttf file and it had the same missalignment as mine. So the question is, if you created the brand.ttf of your package yourself or if it was just a renamed SimpleIcons.ttf of that year (3 years ago).

NikSWE commented 2 years ago

Hi @jlnrrg, I created the brand.ttf file manually using the icomoon platform.

NikSWE commented 2 years ago

I think it would be better to raise this concern on the simpleicons repo, as this issue is in the font file. Could be that they have fixed this issue and not updated the font file.

jlnrrg commented 2 years ago

@NikSWE thanks for the fast answer. Can you go a little bit more into detail if I am not correct? I would assume it was like this:

  1. download all simple-icons svgs
  2. import them all in icomoon
  3. export icomoon as font
NikSWE commented 2 years ago

Correct, this is the process I followed to create the TTF font file

NikSWE commented 2 years ago

you could try one thing just create another font file containing only the apple icon and then import that in the code to see the difference between all the three

jlnrrg commented 2 years ago

Because I am always searching for the correct issue: I escalated the problem back then and now it is opened again at simple-icons. https://github.com/simple-icons/simple-icons-font/issues/113