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

On-demand auth #616

Merged
merged 5 commits into from
Mar 12, 2020
Merged

On-demand auth #616

merged 5 commits into from
Mar 12, 2020

Conversation

akellbl4
Copy link
Collaborator

@akellbl4 akellbl4 commented Mar 11, 2020

This PR solve suggestions form #595 and #426
It is fast realization of on-demand auth.
I can have small bugs and maybe not so clean code.
I will happy with any suggestions connected with realization.

  • auth block was moved to comment form
  • if comments on a page are read-only auth form shows in the old place
  • comment value in the form will be saved between refreshes (it is side effect form saving comment value between an unauth and auth states)
  • sort was moved back to store because now it difficult to pass this value over comments tree

P.S. Auth block needs to refactor and maybe small redesign, I plan to do it in the near feature.

Screenshots

Top comment block:
Screenshot 2020-03-11 at 16 07 37

Comment block under any comment:
Screenshot 2020-03-11 at 16 07 50

Read only page:
Screenshot 2020-03-11 at 16 11 02

* auth block was moved to comment form
* if comments on page read only auth form shows in old place
* comment value in form will be saved between refreshes (it is side effect form saving comment value between unauth and auth states)
paskal
paskal previously approved these changes Mar 11, 2020
Copy link
Collaborator

@paskal paskal left a comment

Choose a reason for hiding this comment

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

The visual part looks good to me, an experience like I would expect from this functionality. Good job!

@umputun
Copy link
Owner

umputun commented Mar 12, 2020

looking good. So far the only thing I run into - image drag-drop (cut-paste) doesn't work in the entry box unless the user logged in. I understand this is due to the required auth for image operation, but user can be surprised. Maybe we should show some message saying "to add image pls login" or smth like this

Mavrin
Mavrin previously approved these changes Mar 12, 2020
Copy link
Collaborator

@Mavrin Mavrin left a comment

Choose a reason for hiding this comment

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

Lgtm

frontend/app/components/auth-panel/auth-panel.tsx Outdated Show resolved Hide resolved
@akellbl4 akellbl4 dismissed stale reviews from Mavrin and paskal via 35bfadd March 12, 2020 08:53
Mavrin
Mavrin previously approved these changes Mar 12, 2020
@umputun
Copy link
Owner

umputun commented Mar 12, 2020

@akellbl4 - do you want me to merge it in or still work in progress?

@akellbl4
Copy link
Collaborator Author

akellbl4 commented Mar 12, 2020

@umputun I think I should add notification about image uploading before merge.

@umputun
Copy link
Owner

umputun commented Mar 12, 2020

I think I should add notification about image uploading before merge.

makes sense.

@umputun
Copy link
Owner

umputun commented Mar 12, 2020

the error message should disappear after the login
grfasmxqhw-20200312-135908

Copy link
Collaborator

@Mavrin Mavrin left a comment

Choose a reason for hiding this comment

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

Please run "npm run generate-langs" and commit changes.

@akellbl4
Copy link
Collaborator Author

@Mavrin done

@umputun umputun merged commit fe4b46f into umputun:master Mar 12, 2020
@umputun umputun added this to the v1.6 milestone Apr 12, 2020
@akellbl4 akellbl4 deleted the on-demand-auth branch June 13, 2021 08:33
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.

4 participants