quran / quran.com-frontend-next

Frontend build on next.js
https://quran.com
1.42k stars 419 forks source link

[bug]: Link to tafsir redirects to homepage on Safari #1630

Open MalaikaAbid opened 2 years ago

MalaikaAbid commented 2 years ago

Is there an existing issue for this?

Describe the bug

When I click a link that looks like it is supposed to access the tafsir section, I am instead redirected to the homepage on Safari.

For example, if I click any of the links in the image below:

Screen Shot 2022-05-05 at 1 14 45 PM

I will be redirected to https://previous.quran.com instead.

This bug was not observed in Chrome. In Chrome, clicking one of the above links takes me to the expected page for tafsir:

Screen Shot 2022-05-05 at 1 18 20 PM

Relevant log output

No response

Steps to reproduce

  1. In Safari, attempt to go to https://quran.com/1:1/tafsirs/en-tafsir-maarif-ul-quran
  2. Observe that you are redirected to https://previous.quran.com.

Environment (please complete the following information) and Add any other context about this bug

OS: macOS Browser: Safari 14.0

Environment

Production(quran.com)

koshpendi commented 2 years ago

i succeeded with https://quran.com/1:1/tafsirs/en-tafsir-maarif-ul-quran

image
MalaikaAbid commented 2 years ago

Ah so looks like it is available to users with a more updated Safari. I updated mine and I can access all the new features now. I wonder if there should be a message to let users know why they are not able to access a feature before it sends them to previous.quran.com, so they aren't spinning their wheels on it like I did. Something like a "Your browser does not support this feature. Please update".

hazem3500 commented 1 year ago

I believe this code is the culprit 👀

https://github.com/quran/quran.com-frontend-next/blob/7514aa3017288b0cc8fccd27ae0b7a3918aab5fe/src/utils/css.ts#L1-L14

If the browser doesn't support logical CSS properties, the app redirects to previous.quran.com 😅

I personally wouldn't have chosen to go that route and would rather transpile logical CSS properties to non-logical ones, but I guess the current solution works 🤷

Though all modern browsers now support CSS logical properties so I guess there is no need for the above ☝️ code anymore 😄

osamasayed commented 1 year ago

I believe this code is the culprit 👀

https://github.com/quran/quran.com-frontend-next/blob/7514aa3017288b0cc8fccd27ae0b7a3918aab5fe/src/utils/css.ts#L1-L14

If the browser doesn't support logical CSS properties, the app redirects to previous.quran.com 😅

I personally wouldn't have chosen to go that route and would rather transpile logical CSS properties to non-logical ones, but I guess the current solution works 🤷

Though all modern browsers now support CSS logical properties so I guess there is no need for the above ☝️ code anymore 😄

@hazem3500, Sadly transpiling logical CSS was not an available option when we used the logical CSS and we would have needed to do it manually which would have been very time consuming. I personally don't like redirecting to previous.quran.com but it's the lesser of two evils :). The docs mention that it's widely supported but after tracking right before we redirect, we have 13000 users monthly who are using browsers that don't support logical CSS. It's not a massive number but without redirecting, the website looks pretty broken to them. Once they upgrade to a new device/browser version, they won't be facing this issue anymore.

hazem3500 commented 1 year ago

Oh, thanks @osamasayed for the clarification 😄

we have 13000 users monthly who are using browsers that don't support logical CSS.

What?! What are these people using, toasters? 😂 Not sure what browsers they're using but 13000 is a significant amount 🤔

Based on this Next.js Doc, couldn't we edit the postcss-config and use the postcss-logical plugin and fix the issue once and for all 👀, then no redirects needed 🤷