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 icon align of outlinks in datatables #16492
Conversation
@sgiehl I think it needs a minor patch to align the icon when using the comparison feature? It seems now to high instead of too low |
Could we backport this fix to 3.x, as it's a visual regression that almost everyone will notice, and some will use 3.x for another year or more? |
@mattab we'll see. We might not even release another 3.X anytime soon and in general it's probably not too important. |
@tsteur is that from the UI tests? Not sure why it's not positioned correctly there. But it looks correct locally in Chrome and Firefox 🤷 |
@sgiehl it looks fine in some reports when comparing (eg pages) but eg in "Websites" there seems to be a slight issue for me in my local install |
4fee828
to
d26fa70
Compare
Ok. Hope everything looks good now. Note: Bandwidth screenshots need to be updated before merging... |
The icons still seem to be a bit off for me eg in websites report @sgiehl ? I made sure the CSS changes were applied by deleting the generated css file unless it's otherwise not applied. Can you have a look again maybe? In pages report it needs to maybe one pixel down as well not sure but noticed when moving 1px it actually does nothing and setting a margin top of 2px is too much again so it might be fine |
@tsteur Then I guess we may need to refactor the whole HTML / CSS around the icons in datatables. Seems the position currently kind of depends on the rendering of the font. That's also the reason why it looks good on my machine, but slightly different in the UI tests. That's how it looks for me on Windows (same for Chrome, Firefox and IE) Could you maybe check how it looks if you remove the apple fonts from the active font list? |
@sgiehl would it help to use font icons? There are indeed differences depending on which font is used. I wonder though why it used to work? There must be something that changed? I suppose could go back in versions to find out which version introduced the regression? I was going to say maybe should no longer use system fonts by default and rather use a font that is available everywhere for a more consistent look across operating systems and browsers but finding a good font that is available everywhere might not be that easy either. |
I don't really think it is possible to use fonts that are the same on every operating system. Without webfonts, there is no font that is available on every OS. And even with webfonts on has the flash during loading of the font. Personally, I also prefer fonts to look like users expect them and choose them (for their environment) rather than the same everywhere. Maybe an alternative would be (inline) SVG files? They are pretty well-supported, but work more like images than fonts. If one doesn't want to duplicate the same SVG over and over, one could use defs and use like this: https://css-tricks.com/lodge/svg/13-svg-icon-system-use-element/ |
44a68fd
to
577a1d2
Compare
I've now included the icons in the Matomo font. Hope it now also works on a mac. |
@sgiehl on Mac - Chromium it's now a pixel or two too high but at least the behaviour seems to be consistent across all reports, comparison feature, subtables. Would still need to move the icon though Might also be good to use browserstack if needed? |
@tsteur It still looks good on my side. So maybe it's still font related 🤷 |
Sent you the login data @sgiehl maybe you can have a look there? Wonder though how it used to work. There must have been some change? |
da62066
to
a45b86c
Compare
dd38e36
to
c0d487a
Compare
@sgiehl great. It looks definitely better now and great to see icons there instead of images. The plus/minus icon looks still 1px too high. This is also the case for me for all other fonts except for "sans-serif". Not sure what font Mac than is falling back to. I see though in the screenshots it seems to look fine (eg in https://media.githubusercontent.com/media/matomo-org/matomo/0e4626bf289fd16172ccbcbb787fe6923da34bef/plugins/Actions/tests/UI/expected-screenshots/ActionsDataTable_pageview_percentages.png). Although maybe it could also be moved one pixel further down if any possible? (In icomoon we could if needed move the icon one pixel down when generating the font AFAIK). It's nicely aligned in the tests but wonder if it was possible to move one pixel down because mostly we would be having maybe lower case and the plus is quite a bit bigger than the lower letter? Don't know what it looks like though on Linux/Windows. fyi on mac with upper case letters when using sans serif then it looks really well on Mac and wouldn't want to have it moved a pixel down. I assume this is the case for Windows/Linux too? So maybe we wouldn't want to move it down a pixel. |
@tsteur I've moved the icons a pixel down in the font... Let me know if it's good to merge. Would then update the expected screenshots in the submodules before merging... |
@sgiehl I'm still kind of having the same issue (100% cache was not used and using updated file). I think it moved it maybe slightly down. Like when I zoom in I can see at least the plus is on the same line as the font but when viewing it in original size it's not 100% correct and still bit too high. I don't know if it can be moved again a bit further down or so? I wonder actually if a different font has a better icon. This icon doesn't seem to be as much suited for being displayed so small. Even on a retina display it looks a bit pixelated due to some aliasing or so On my regular screen the icon looks quite blurry It's not as much visible in a screenshot I think. Other fonts might have the same problem though considering we show it so small (and my regular screen is not the best but users might have similar screens). Tested some other icons like from material design but they have issue too see above regular screen vs below retina (looking a lot better) Is there maybe some font smoothing or so that can be disabled to make this better or maybe something needs to be enabled? Funny thing: The old image that we were using looks good on my regular screen but bad on the retina screen :) It might not be too important for now just thought I mention it. I don't know what it looks like on Windows/Linux and whether it can be again moved down another pixel the plus / minus icon. The outlink is fine and not needs changing |
On windows the align is actually fine for the plus/minus icon. I've added some additional css for the icon to improve the smoothing... Can't see a difference on Windows, but maybe it helps for Mac |
Doesn't really change anything here. I'm thinking the icon might not be best but I guess most other icons would have same problem since it's shown so small. Could we maybe show the icon bigger? Like below ( |
increasing the icon size sounds fine. will update the css and expected screenshots... |
That works well on Mac 👍 not sure what it's like on Windows or Linux but for me it looks good to merge |
fixes #16436