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

Safari iOS tag alignment patch #155

Closed
wants to merge 2 commits into from
Closed

Safari iOS tag alignment patch #155

wants to merge 2 commits into from

Conversation

zerosonesfun
Copy link
Contributor

@zerosonesfun zerosonesfun commented Jan 20, 2022

In Safari iOS, child tags are slightly higher than the parent tag.

I’m recommending adding this at the bottom of this file with the comment left in place so that it can be easily removed in the future as needed.

Also, to be safe, using .mobile-safari class which is already automatically added to the #app class list when Safari iOS is detected. Thus, fix/patch will not affect other browsers. 🤦‍♂️This isn’t true. The mobile safari class I saw was added by my dev testing browser and not Flarum. This fix still works though! I just need help throughly testing because I only have a Mac and iPhone.

**Fixes #3245 **

Changes proposed in this pull request:
This adds a CSS rule for Safari iOS. Fixes a tag alignment issue only in Safari iOS. Might not always be needed, therefore added to bottom of file with CSS comment.

Reviewers should focus on:
You need an iPhone to test. It’s the only way. Or an accurate emulator. You should see that without this fix, certain secondary tags are not aligned.

Then, since I only have a Mac and iPhone, someone needs to test this on a PC and Android device. For this test you’re looking to ensure nothing changes. Rule should only help iPhone, and not affect other devices/browsers.

Screenshot
Fixes this. Look closely. 👀

screenshot

Necessity

  • Has the problem that is being solved here been clearly explained?
  • If applicable, have various options for solving this problem been considered?
  • For core PRs, does this need to be in core, or could it be in an extension?
  • Are we willing to maintain this for years / potentially forever?

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).
  • Core developer confirmed locally this works as intended.
  • Tests have been added, or are not appropriate here.

Required changes:

  • Related documentation PR: (Remove if irrelevant)
  • Related core extension PRs: (Remove if irrelevant)

In Safari iOS, child tags are slightly higher than the parent tag.

I’m recommending adding this at the bottom of this file with the comment left in place so that it can be easily removed in the future as needed. 

Also, to be safe, using .mobile-safari class which is already automatically added to the #app class list when Safari iOS is detected. Thus, fix/patch will not affect other browsers.
@zerosonesfun
Copy link
Contributor Author

zerosonesfun commented Jan 22, 2022

I originally added a special Safari class thinking Flarum added it. But, that was a special browser I was using that added that. 🤦‍♂️

My fix still works, but I need to remove the .mobile-safari class.

@zerosonesfun
Copy link
Contributor Author

zerosonesfun commented Jan 22, 2022

Ok, edited original pull request and removed .mobile-safari. Again, 🤦‍♂️.

Of course another option since this is just one extra CSS rule and maybe temporary, is to close this pull request and a core developer can just add the CSS.

It’s simply:

.TagsLabel > * {
vertical-align: bottom;
}

@askvortsov1
Copy link
Sponsor Member

Ok, edited original pull request and removed .mobile-safari. Again, man_facepalming.

Of course another option since this is just one extra CSS rule and maybe temporary, is to close this pull request and a core developer can just add the CSS.

It’s simply:

.TagsLabel > * {
vertical-align: bottom;
}

It actually is a Flarum thing! https://github.com/flarum/core/blob/19e48617f0ae358f94396989b3855ce5294c4951/js/src/forum/ForumApplication.ts#L135-L139

I think I would be fine approving this if it had the mobile-safari requirement again, to constrain potential side effects.

@zerosonesfun
Copy link
Contributor Author

Well then, I’m just all confused.

It didn’t work consistently with the mobile safari constraint. That’s why I assumed it maybe wasn’t an official part of Flarum.

I think I’ll start over later. As in, test out other options besides .TagsLabel > *.

I’ll close this until I have something more solid.

@zerosonesfun
Copy link
Contributor Author

@askvortsov1 Now I'm glad I went ahead and closed this. Just realized it's not just mobile safari, it is safari in general. Therefore even if it worked, using the .mobile-safari class route won't work. womp, womp :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants