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

Change add feed action #3027

Merged
merged 2 commits into from
Jun 5, 2020
Merged

Conversation

aledeg
Copy link
Member

@aledeg aledeg commented Jun 2, 2020

Closes #777

Changes proposed in this pull request:

  • Add a dedicated page to add a feed to avoid multiple clicks.
  • Change the subscription page by removing the add feed action and reducing the size of the add category action.
  • Change main page to add direct access to the add feed action.

How to test the feature manually:

  1. Add a simple feed
  2. Add a category
  3. Add a feed with a new category
  4. Add a feed with HTTP authentication

Pull request checklist:

  • clear commit messages
  • code manually tested
  • unit tests written (optional if too hard)
  • documentation updated

Additional information can be found in the documentation.

To ease the use of this feature, there is a new dedicated page to avoid numerous clicks.
Especially for switching categories or using HTTP authentication. The underlying actions
remain identical but the interface is simplified.

@aledeg
Copy link
Member Author

aledeg commented Jun 2, 2020

UX is definitely not my strong suit so please guide me to have something usable

@Alkarex Alkarex added this to the 1.17.0 milestone Jun 2, 2020
@Alkarex
Copy link
Member

Alkarex commented Jun 2, 2020

I will check more tonight, but looks good 👍
Maybe you could add a few more advanced parameters, which otherwise might prevent the feed from being added (e.g. timeout, SSL):
image

Furthermore, maybe we should have a checkbox for adding #force_feed

There is a minor layout glitch in at least one theme:
image

@mlaily
Copy link
Contributor

mlaily commented Jun 2, 2020

If you want feedback, here is mine, as the original issue creator.

I like the dedicated "Add a feed" page. 🙂
The option to create a new category directly from this page is not necessary in my opinion, bu it's really not a problem.

I find the "Subscription management" page a bit misleading now, because the only action is the creation of a category, which is rarely used.
It's also immediately under the "Subscription management" header, and not clearly separated from the feeds. (It was in some kind of titled box before. Did you change that because it was taking too much space?)

  • Maybe add a link/button to the feed creation page? Or maybe even a simple line of text indicating there is a dedicated page to subscribe to a feed? (I know it's also in the left menu, but I think it would help to see it on the page, especially for long time user that expect to find the feed subscription button here)
  • Maybe add a legend or some kind of header to the category creation? I think it would be better to have something like "Add/Create a new category" instead of just the watermark in an isolated text input. It could stay all on one like if you think it's better.
  • Maybe add some kind of separation (a simple <hr/> ?) between the new category input and the feeds list.

Sorry for the lengthy feedback. I might be a bit too nit-picking, but it could be the only chance I get in 5 years, so I hope you forgive me. 😀

Thank you for your work.

@aledeg
Copy link
Member Author

aledeg commented Jun 2, 2020

@Alkarex I do not have this problem on my machine. I am using FF on Arch. Even on smaller screens, there is no such thing.
I do not want to include more fields since they are not required to work. Basically, I've only moved the original form in its own page. This new page is only meant to add the feed. Once it's done, you are automatically redirected to its configuration where you can tweak the feed configuration.

@mlaily I hear you. I've moved the category creation in the feed creation page where I've separated the 2 forms. I've also renamed the page to show that there is one page to create the elements.
I've also added a small information box containing a link to the new page.

@aledeg aledeg force-pushed the feature/subscription-process branch 5 times, most recently from ec9d380 to 22d79a3 Compare June 2, 2020 18:46
@mlaily
Copy link
Contributor

mlaily commented Jun 2, 2020

I like your changes.

Something seems broken when adding a feed though.

image

172.17.0.1 - - [02/Jun/2020:19:14:25 +0000] "GET /i/?c=subscription&a=add HTTP/1.1" 200 2672 "-" "Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Firefox/78.0"
[Tue Jun 02 19:14:34.139830 2020] [php7:notice] [pid 18] [client 172.17.0.1:41786] PHP Notice:  Undefined variable: Minz_Request in /var/www/FreshRSS/app/Controllers/feedController.php on line 166
[Tue Jun 02 19:14:34.139936 2020] [php7:error] [pid 18] [client 172.17.0.1:41786] PHP Fatal error:  Uncaught Error: Class name must be a valid object or a string in /var/www/FreshRSS/app/Controllers/feedController.php:166\nStack trace:\n#0 /var/www/FreshRSS/lib/Minz/Dispatcher.php(119): FreshRSS_feed_Controller->addAction()\n#1 /var/www/FreshRSS/lib/Minz/Dispatcher.php(47): Minz_Dispatcher->launchAction('addAction')\n#2 /var/www/FreshRSS/lib/Minz/FrontController.php(84): Minz_Dispatcher->run()\n#3 /var/www/FreshRSS/p/i/index.php(48): Minz_FrontController->run()\n#4 {main}\n  thrown in /var/www/FreshRSS/app/Controllers/feedController.php on line 166
172.17.0.1 - - [02/Jun/2020:19:14:33 +0000] "POST /i/?c=feed&a=add HTTP/1.1" 200 336 "-" "Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Firefox/78.0"

@aledeg
Copy link
Member Author

aledeg commented Jun 2, 2020

Thank you. I'll look at it

@aledeg aledeg force-pushed the feature/subscription-process branch from 22d79a3 to 276d08f Compare June 2, 2020 19:41
@aledeg
Copy link
Member Author

aledeg commented Jun 2, 2020

@mlaily it's fixed.

@mlaily
Copy link
Contributor

mlaily commented Jun 2, 2020

I did some more testing.

As I said, I like it like that. The only thing I find strange is that validation (popup messages for an invalid url, an empty category name...) is done on the subscriptions page, and not the page where the inputs are now...

@aledeg
Copy link
Member Author

aledeg commented Jun 2, 2020

Good catch! I didn't noticed before. I'll look into it

@aledeg aledeg force-pushed the feature/subscription-process branch 2 times, most recently from 13a96b3 to 5b34650 Compare June 2, 2020 20:00
@aledeg
Copy link
Member Author

aledeg commented Jun 2, 2020

@mlaily it's fixed!

@mlaily
Copy link
Contributor

mlaily commented Jun 2, 2020

That was quick! :) Everything looks fine by me.

@aledeg aledeg force-pushed the feature/subscription-process branch from 5b34650 to d5a9f91 Compare June 2, 2020 21:13
@Alkarex
Copy link
Member

Alkarex commented Jun 2, 2020

@aledeg For the layout bug, try with Pafat theme in English.
image
or Blue Lagoon
image

To solve the bug and simplify the UI, I think you could consider dropping one of the icons (e.g. the one for import/export, and keep only the one to add a new feed).

For the additional fields, it can also wait for another PR, because it is not something we have now anyway. The reason for exposing timeout and SSL (and #force_feed) at subscription time is that otherwise it is not possible to add a feed that needs those parameters without changing global parameters, because adding the feed will fail before allowing the user to change those parameters.

See this example of feed that takes 30s to load http://test.alapetite.fr/slow.rss.xml.php and try to add it to FreshRSS (which, by default, has 15s timeout)

Minor: Maybe "Add" instead of "Create" for the button label:
image
(It took be a couple of seconds to understand where I should click to proceed with adding the feed)

@aledeg aledeg force-pushed the feature/subscription-process branch 3 times, most recently from b4cc4c7 to 5722cb0 Compare June 2, 2020 21:49
@aledeg
Copy link
Member Author

aledeg commented Jun 2, 2020

@Alkarex I've removed the import button and change the action button text. For the other fields, I'll rework it when they are available.

@Alkarex
Copy link
Member

Alkarex commented Jun 2, 2020

@aledeg The other fields are already available, but currently only from a feed configuration page (so we have the text, parameters, etc.)

@aledeg
Copy link
Member Author

aledeg commented Jun 2, 2020

Ok, I'll look into it tomorrow.

@aledeg aledeg force-pushed the feature/subscription-process branch from 5722cb0 to 8deb21d Compare June 3, 2020 06:13
@aledeg
Copy link
Member Author

aledeg commented Jun 3, 2020

@Alkarex I've added timeout and ssl. I could not find the force option.

https://alpinelinux.org/posts/Alpine-3.12.0-released.html
With PHP 7.3.18 (from 7.3.17) (and Apache 2.4.43 unchanged).
No other significant change spotted
@aledeg aledeg force-pushed the feature/subscription-process branch from 8deb21d to 3fd94e7 Compare June 3, 2020 18:01
@Alkarex
Copy link
Member

Alkarex commented Jun 3, 2020

The feed attributes were not applied / used before loading. Fixed by 52ba7ff

@Alkarex
Copy link
Member

Alkarex commented Jun 3, 2020

@marienfressinaud Do you want to have a look at the new UI?

@aledeg aledeg force-pushed the feature/subscription-process branch from 52ba7ff to 424c823 Compare June 3, 2020 18:28
Copy link
Member

@marienfressinaud marienfressinaud left a comment

Choose a reason for hiding this comment

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

Maybe the "login" and "advanced" sections can be merged together, and hidden by default? It looks like they are required and make the form more complex than it is

app/views/subscription/add.phtml Outdated Show resolved Hide resolved
@marienfressinaud
Copy link
Member

I don't have a lot of time this week to spend on the review but it's a nice improvement and I trust you for the rest of the review :)

@aledeg aledeg force-pushed the feature/subscription-process branch from 424c823 to cd7bbf7 Compare June 5, 2020 06:14
@aledeg
Copy link
Member Author

aledeg commented Jun 5, 2020

@marienfressinaud the idea is to keep the layout from the add page and the layout from the update panel with the same fields and headers organization. For sure, the add page is a skimmed version of the update page but I find that hiding elements will be counter-intuitive.

@Alkarex Alkarex merged commit d4554fa into FreshRSS:master Jun 5, 2020
@aledeg aledeg deleted the feature/subscription-process branch June 5, 2020 08:16
Alkarex added a commit to Alkarex/FreshRSS that referenced this pull request Jul 6, 2020
Alkarex added a commit that referenced this pull request Jul 6, 2020
@Alkarex Alkarex modified the milestones: 1.17.0, 1.16.3 Aug 29, 2020
aledeg added a commit to aledeg/FreshRSS that referenced this pull request May 31, 2021
Before, the links was redirecting to the subscription management page which
was the default behavior before changes introduced in 1.17.0 (FreshRSS#3027). All
links were modified except the one for empty content.
Now, the empty content links are redirecting to the proper page.

See FreshRSS#3642
@aledeg aledeg mentioned this pull request May 31, 2021
4 tasks
Alkarex pushed a commit that referenced this pull request May 31, 2021
Before, the links was redirecting to the subscription management page which
was the default behavior before changes introduced in 1.17.0 (#3027). All
links were modified except the one for empty content.
Now, the empty content links are redirecting to the proper page.

See #3642
@math-GH math-GH mentioned this pull request Nov 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UX très frustrante pour l'ajout d'un nouveau flux
4 participants