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 (BB-456) : CSS style issue in older version of certain browsers #559

Merged
merged 6 commits into from
Mar 25, 2021

Conversation

akashgp09
Copy link
Contributor

Signed-off-by: Akash Gupta akashgp9@gmail.com

Problem

Some Older version of CSS does not support certaion CSS properties

Solution

By adding vendor pefix we can tackle this

Areas of Impact

CSS Styles

Signed-off-by: Akash Gupta <akashgp9@gmail.com>
@welcome
Copy link

welcome bot commented Mar 11, 2021

Thanks for opening your first pull request for BookBrainz, and welcome to our community! 🎉

If you haven't yet, please check out our contributing guidelines.

Someone will be reviewing your PR soon; just hang in there !
In the meantime, if you're wondering what to do next, you can have a look at our issue tracker

@coveralls
Copy link

coveralls commented Mar 11, 2021

Coverage Status

Coverage increased (+0.01%) to 60.935% when pulling e95af12 on akashgp09:style-issue into f536747 on bookbrainz:master.

Copy link
Contributor

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @akashgp09 !
Thanks for opening this PR !

I think you have some form of auto-formatting set up for your code editor, that has modified the file a lot and makes it pretty hard to review, especially for a change of a few lines.

Could I ask you to redo it with the auto-formatting disabled in your editor?

You should be able to checkout the version of the less file from the master branch with git checkout master -- src/client/stylesheets/style.less, reapply your changes to the .deleted class and commit again, without having to open another PR.

Have you been able to test in the older browser versions mentioned in the ticket?

@akashgp09
Copy link
Contributor Author

Could I ask you to redo it with the auto-formatting disabled in your editor?

sure @MonkeyDo, I have reverted the prettier formatting :)

Have you been able to test in the older browser versions mentioned in the ticket?

I didn't tested it. But i read few article for the solution and also looked into the MDN docs, it was suggested that using the vendor prefix can solve some css issue for older browser versions

Copy link
Contributor

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This hasn't fixed the issue on the browsers mentioned in the ticket. You can test using BrowserStack for example: https://www.browserstack.com

I suppose it doesn't hurt to keep the prefixes you added if it makes it work in some browsers we don't know about.

However I realized while checking browser support on can I use that the css property text-decoration: line-through; will work for all the browsers who don't support text-decoration-line
https://caniuse.com/text-decoration says

All browsers support the CSS2 version of text-decoration, which matches only the text-decoration-line values (underline, etc.)

So that seems like a good solution to add along with the existing css as a fallback.
However that strikethrough is the same grey color as the text, so maybe there's something there to investigate to see if it can be changed: https://developer.mozilla.org/fr/docs/Web/CSS/text-decoration

src/client/stylesheets/style.less Outdated Show resolved Hide resolved
Copy link
Contributor

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still not working as expected on the browsers I tried.
Please do test that your solution works; I know in this case testing on ancient browsers seems really tricky, but you can use BrowserStack with a free account to visit the website on various browsers (specifically the browsers listed in the ticket).

What I then do to test is visit a deleted entity and use whatever developer tools there are on that old browser to inspect and modify the css, to see what works/doesn't work.

That way you'll be able to find a solution that works everywhere, with progressive fallbacks :)

src/client/stylesheets/style.less Outdated Show resolved Hide resolved
src/client/stylesheets/style.less Outdated Show resolved Hide resolved
Copy link
Contributor

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a great solution, nice and simple, well done !

@MonkeyDo MonkeyDo merged commit d4648cf into metabrainz:master Mar 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants