Skip to content

Improved: Extension manager: new style#4181

Merged
Alkarex merged 33 commits intoFreshRSS:edgefrom
math-GH:extension-manager-news-style
Mar 14, 2022
Merged

Improved: Extension manager: new style#4181
Alkarex merged 33 commits intoFreshRSS:edgefrom
math-GH:extension-manager-news-style

Conversation

@math-GH
Copy link
Copy Markdown
Contributor

@math-GH math-GH commented Jan 30, 2022

Before:
grafik

It has some issues:

  1. on wide screens the text is far away from the buttons (see screenshot on a not very wide screen)
  2. on mobile screen the text overlaps the buttons (see next screenshot)
  3. the middle button is a status button. The status is less clear (the text disable/enable is not the status. It is the action when the user clicks on it).
  4. the config button (cog icon) is in most screens in FreshRSS very clear. It does not need the text "manage"
  5. the remove button is not very often used but very prominent placed, but not in the extension manage slider

grafik

After:
grafik

  1. fixed. Text is always close to the buttons
  2. fixed. No text overlap anymore
  3. prominent status (I call it "switch"). Status is very clear.
  4. simple manage/config button
  5. remove button is removed/moved to the config slider (see screenshot below)

grafik

Other themes:
Alternative dark theme
grafik

Dark theme
grafik

Changes proposed in this pull request:

  • better HTML
  • a lot of CSS

How to test the feature manually:

  1. go to the extension configuration
  2. see also the nice mouse hover on the switches

Pull request checklist:

  • clear commit messages
  • code manually tested
  • CSS: RTL
  • CSS: styling the other themes
  • solved the question: Show system extensions to user?

@math-GH
Copy link
Copy Markdown
Contributor Author

math-GH commented Jan 30, 2022

It is a draft PR to collect your first feedback

@math-GH math-GH added Theme 🖌 UI 🎨 User Interfaces labels Jan 30, 2022
@math-GH math-GH added this to the 1.20.0 milestone Jan 30, 2022
@Alkarex
Copy link
Copy Markdown
Member

Alkarex commented Jan 30, 2022

Looks very nice so far 👍🏻
No change in JavaScript requirements?


<?php if (FreshRSS_Auth::hasAccess('admin')) { ?>
<form id="form-extension" method="post">
<input type="hidden" name="_csrf" value="<?= FreshRSS_Auth::csrfToken() ?>" />
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Indentation's a bit weird here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed.

Copy link
Copy Markdown
Member

@Frenzie Frenzie left a comment

Choose a reason for hiding this comment

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

@math-GH
Copy link
Copy Markdown
Contributor Author

math-GH commented Jan 30, 2022

wow, you are very very fast :)
I had a bug, so that the circle was not on the correct position, when it is enabled. Fixed it (also the screenshots are now fixed)

@math-GH
Copy link
Copy Markdown
Contributor Author

math-GH commented Jan 30, 2022

Looks good to me in principle. Also note https://codepen.io/Qvcool/pen/bdzVYW

I tried also some different styles, but I do not like this kind of switches, because the status there is not 100% clear (for me. I need to think longer. For me it is a strange UX)

@math-GH
Copy link
Copy Markdown
Contributor Author

math-GH commented Jan 30, 2022

No change in JavaScript requirements?

Zero JavaScript here. The "disable/enable" button got just a new style. The logic behind the button was not changed.

@Frenzie
Copy link
Copy Markdown
Member

Frenzie commented Jan 30, 2022

I tried also some different styles, but I do not like this kind of switches, because the status there is not 100% clear (for me. I need to think longer. For me it is a strange UX)

I'm not really sure what you're talking about? I was saying these do the same thing without introducing svg images.

@math-GH
Copy link
Copy Markdown
Contributor Author

math-GH commented Jan 30, 2022

I tried also some different styles, but I do not like this kind of switches, because the status there is not 100% clear (for me. I need to think longer. For me it is a strange UX)

I'm not really sure what you're talking about? I was saying these do the same thing without introducing svg images.

Thanks for the explanation. Now I see what you mean. I meant another topic ;)
I like this non-svg solution much more. Will implement it. Thanks for this hint.

@math-GH
Copy link
Copy Markdown
Contributor Author

math-GH commented Jan 30, 2022

@Frenzie : It is made with Font Awesome icons. I am not a fan of it.
I will keep the SVG solution.

grafik

@Frenzie
Copy link
Copy Markdown
Member

Frenzie commented Jan 30, 2022

Font Awesome is an irrelevant detail. Pretend it says something like or or 🗸 for example instead of Font Awesome. ;-)

Btw, not shown on the screenshots is that these checkbox buttons are currently a bit too jumpy.

@math-GH
Copy link
Copy Markdown
Contributor Author

math-GH commented Jan 30, 2022

Btw, not shown on the screenshots is that these checkbox buttons are currently a bit too jumpy.

Do you mean this animation?
switch-animatoin
(information: the gif animation has an bug. Issue: The second switch from the bottom does not hide the icon in the animation)

@Frenzie
Copy link
Copy Markdown
Member

Frenzie commented Jan 30, 2022

Sidenote, Netsurf works for me but colorblind people would prefer a regular checkbox. But I'm not sure if or to what extent we should worry about that. ^-^

Screenshot_20220130_160609

@Frenzie
Copy link
Copy Markdown
Member

Frenzie commented Jan 30, 2022

Do you mean this animation?

Yes, I'd prefer to do without the horizontal sliding movements.

@math-GH
Copy link
Copy Markdown
Contributor Author

math-GH commented Jan 30, 2022

Sidenote, Netsurf works for me but colorblind people would prefer a regular checkbox. But I'm not sure if or to what extent we should worry about that. ^-^

It is not a checkbox. It is still the same button as before (click on it and it will reload the page).

I would say that Netsurf and color blind people are not a good combination.

Disabled extensions have an italic text style.

@Frenzie
Copy link
Copy Markdown
Member

Frenzie commented Jan 30, 2022

It is not a checkbox. It is still the same button as before (click on it and it will reload the page).

It's a sliding checkbox. :-) It could just as well be represented as a regular checkbox or as a fake checkbox that is compatible with Netsurf. For example, if you add an enabled/disabled SVG icon much like the one proposed here (but represented by Unicode for the sake of putting it together in 10 seconds):

Screenshot_20220130_164055

I would say that Netsurf and color blind people are not a good combination.

Maybe, maybe not, but the point is that it wasn't an issue before.

@math-GH
Copy link
Copy Markdown
Contributor Author

math-GH commented Jan 30, 2022

@Frenzie : Please check with Netsurf: What is not supported by Netsurf? ::before/::after or content:

@Frenzie
Copy link
Copy Markdown
Member

Frenzie commented Jan 30, 2022

Curiously, I see references to :before/:after in the source code but the answer seems to be a simple yes to both.

Anyway, there's no need to worry too much about it, but @Alkarex reminded us recently that we've been forgetting to test in Netsurf and friends. ^_^

There is a standard way to work around the issue with @supports btw.

Something like (spitballin', I'd say it's better to use for example an inner span with some text to display:none so it also works as expected in Lynx/Elinks/Links2):

diff --git a/p/themes/base-theme/template.css b/p/themes/base-theme/template.css
index 28756c87..1c41cb8d 100644
--- a/p/themes/base-theme/template.css
+++ b/p/themes/base-theme/template.css
@@ -335,6 +335,18 @@ a.btn {
        background-color: #eee;
 }
 
+.switch {
+       background-image: url('../icons/disabled.svg');
+}
+.switch.active {
+       background-image: url('../icons/enabled.svg');
+}
+@supports selector(.switch::before) {
+       .switch, .switch.active {
+               background-image: none;
+       }
+}
+
 .switch.active::before {
        background-image: url('../icons/enabled.svg');
        background-position: center center;

@math-GH
Copy link
Copy Markdown
Contributor Author

math-GH commented Jan 30, 2022

I know @supports. That is why I ask, I would try it similar to your example. But do not forget: grey icon on a green background is for red-green blind people grey on grey ;)

@Frenzie
Copy link
Copy Markdown
Member

Frenzie commented Jan 30, 2022

Like I said, just showing the general concept. It's fugly as written in that poc regardless of contrast, lol.

@Frenzie
Copy link
Copy Markdown
Member

Frenzie commented Feb 5, 2022

That's very strange indeed, since I posted a screenshot from 3.10 as well. Oh well. :-)

@math-GH math-GH marked this pull request as draft February 5, 2022 17:08
@math-GH
Copy link
Copy Markdown
Contributor Author

math-GH commented Feb 5, 2022

Does it make sense to display the system extensions to non-admin users?

(screenshot from "before the PR")
grafik

There are 2 possible improvements:

  1. do not display system extensions at all, or
  2. display the extensions and show the status (enabled/disabled)

@math-GH math-GH marked this pull request as ready for review February 6, 2022 10:16
@math-GH
Copy link
Copy Markdown
Contributor Author

math-GH commented Feb 22, 2022

Does it make sense to display the system extensions to non-admin users?
There are 2 possible improvements:

1. do not display system extensions at all, or

2. display the extensions and show the status (enabled/disabled)

I chosed 2

@Alkarex Alkarex merged commit 7b962e2 into FreshRSS:edge Mar 14, 2022
@math-GH math-GH deleted the extension-manager-news-style branch March 14, 2022 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Theme 🖌 UI 🎨 User Interfaces

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants