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 favorite display for "current" articles #2634

Merged

Conversation

marienfressinaud
Copy link
Member

Favorite articles have, in most of the themes, a gold background to
distinguish them from the other articles. However, it can be distracting
to have such a background when reading the articles, so we should turn
them back to the "default" background when articles are opened (class
.current).

Bug introduced in e9ce27d

I didn't apply the fix to the Ansum and Mapco designs because they
already had this behaviour before the commit.

Related PRs:

Closes #2618

@Frenzie
Copy link
Member

Frenzie commented Nov 5, 2019

I didn't apply the fix to the Ansum and Mapco designs because they
already had this behaviour before the commit.

I very much doubt it's on purpose. I'd change it. :-)

Anyway, looks fine. Did you also test the hover behavior? It doesn't really matter if you didn't since it'll be a positive commit either way, but otherwise someone would have to do a minor follow-up still. (I already volunteered myself a few days ago.)

@Frenzie Frenzie added this to the 1.15.1 milestone Nov 5, 2019
@marienfressinaud marienfressinaud force-pushed the fix/2618/favorite-article-display branch from 7312089 to 01ce2be Compare November 5, 2019 13:09
@Alkarex Alkarex added the UI 🎨 User Interfaces label Nov 5, 2019
@marienfressinaud marienfressinaud force-pushed the fix/2618/favorite-article-display branch from 01ce2be to 8dcf20e Compare November 5, 2019 13:47
@marienfressinaud
Copy link
Member Author

I fixed Ansum and Mapco as well.

I wasn't sure of what should be the behaviour on hover: for the themes I patched, there is no change when the article is unread or favorite. I find it better for Ansum and Mapco because the background of the line and the background of the title are a bit de-synchronized due to the animation (at least on Firefox)

Favorite articles have, in most of the themes, a gold background to
distinguish them from the other articles. However, it can be distracting
to have such a background when reading the articles, so we should turn
them back to the "default" background when articles are opened (class
`.current`).

Bug introduced in e9ce27d

Related PRs:

- FreshRSS#2477
- FreshRSS#2611
- FreshRSS#2612

Closes FreshRSS#2618
@marienfressinaud marienfressinaud force-pushed the fix/2618/favorite-article-display branch from 8dcf20e to 98f717f Compare November 5, 2019 13:51
@Frenzie
Copy link
Member

Frenzie commented Nov 5, 2019

I wasn't sure of what should be the behaviour on hover: for the themes I patched, there is no change when the article is unread or favorite.

Whatever the behavior was in 1.14 should be fine. The most important thing is that you don't want to see anything ugly like this on hover:

Screenshot_2019-11-05_15-52-06

@marienfressinaud
Copy link
Member Author

Yep, I fixed that :)

@marienfressinaud marienfressinaud merged commit bba0b05 into FreshRSS:dev Nov 5, 2019
@marienfressinaud marienfressinaud deleted the fix/2618/favorite-article-display branch November 5, 2019 15:43
@Alkarex Alkarex mentioned this pull request Nov 6, 2019
javerous pushed a commit to javerous/FreshRSS that referenced this pull request Jan 20, 2020
Favorite articles have, in most of the themes, a gold background to
distinguish them from the other articles. However, it can be distracting
to have such a background when reading the articles, so we should turn
them back to the "default" background when articles are opened (class
`.current`).

Bug introduced in e9ce27d

Related PRs:

- FreshRSS#2477
- FreshRSS#2611
- FreshRSS#2612

Closes FreshRSS#2618
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
UI 🎨 User Interfaces
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants