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

feat: Add Listed pane in preferences #651

Merged
merged 20 commits into from
Sep 29, 2021
Merged

feat: Add Listed pane in preferences #651

merged 20 commits into from
Sep 29, 2021

Conversation

amanharwara
Copy link
Member

This PR adds a new preferences pane for integration with Listed. When there are no current Listed installations, the pane shows information about Listed and how to get started:

Listed-Pane-Empty

When there are present Listed installations, it shows a list of the blogs with options to open the blog, open the settings and disconnect the blog:

Listed-Pane-WithItems

When the disconnect button is pressed, it shows a confirmation dialog before disconnecting:

Listed-Pane-Disconnect

@moughxyz
Copy link
Member

@amanharwara please pull in standardnotes:develop to your branch to resolve conflicts :)

Copy link
Contributor

@gorjan5sk gorjan5sk left a comment

Choose a reason for hiding this comment

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

Some small finesse changes needed for code consistency and type-safety, otherwise good work! :)

app/assets/javascripts/preferences/panes/Listed.tsx Outdated Show resolved Hide resolved
refactor: Use FunctionalComponent type
@amanharwara
Copy link
Member Author

Made the change from react to preact hooks and FunctionalComponent instead of JSXInternal.Element.

app/assets/javascripts/preferences/panes/Listed.tsx Outdated Show resolved Hide resolved
<Button
type="danger"
label={isDisconnecting ? 'Disconnecting...' : 'Disconnect'}
disabled={disabled}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could use the isDiconnecting state variable here and get rid of the isDeleting one on the parent component

Copy link
Member Author

Choose a reason for hiding this comment

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

Since the deletion happens asynchronously, I've kept the isDeleting on the parent so the other Disconnect buttons can stay disabled while one is disconnecting to avoid any errors.

if (shouldDisconnect) {
disconnect(item);
} else {
setIsDisconnecting(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

setIsDisconnecting(false) would need to go in a finally block to ensure it executes even if there's an error

Copy link
Member Author

Choose a reason for hiding this comment

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

Putting it in the finally block turns it off even before it finishes disconnecting, since the deleteItem() function is async. I've just put setIsDisconnecting(false) in the catch block to execute it even if there is an error.

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we await disconnect in that case? Might be a good idea to refactor these to use async/await which is a bit easier to read I think

Copy link
Member Author

Choose a reason for hiding this comment

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

That would require the disconnect function to be promisified. That is actually how I had originally implemented it, but I thought I was overcomplicating it, so I removed the promise.

Copy link
Member Author

Choose a reason for hiding this comment

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

@antsgar So I've turned the disconnect function into a Promise and using await disconnect() in the BlogItem and setIsDisconnecting() inside the .finally() block.

@antsgar
Copy link
Contributor

antsgar commented Sep 28, 2021

@amanharwara really nice job! 🙂

@amanharwara
Copy link
Member Author

Hi everyone,
I reckon with the latest commit, this PR should be ready for merging.
Can someone re-review?

@moughxyz moughxyz merged commit e72d737 into standardnotes:develop Sep 29, 2021
@amanharwara amanharwara deleted the feat/prefs-listed branch September 29, 2021 16:51
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