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

Added read mode #523

Merged
merged 6 commits into from
Sep 12, 2024
Merged

Added read mode #523

merged 6 commits into from
Sep 12, 2024

Conversation

acaldas
Copy link
Member

@acaldas acaldas commented Sep 11, 2024

No description provided.

@acaldas acaldas self-assigned this Sep 11, 2024
Copy link

vercel bot commented Sep 11, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
connect ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 12, 2024 10:37am

Copy link

@ryanwolhuter ryanwolhuter left a comment

Choose a reason for hiding this comment

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

Looks great,

I just saw that the default editor text is not center aligned anymore. Made a small PR to fix it. I can merge it now and then you can include it in this PR.

powerhouse-inc/design-system#365

<PHLogo />
</ModalManager>
</UiNodesContextProvider>
<ReadModeContextProvider>
Copy link
Member Author

Choose a reason for hiding this comment

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

Explored using a context instead of adding to the useDocumentDrive hook.

const { customEditorLoader } = props;
const [showLoading, setShowLoading] = useState(false);

// only shows the loader after some time has passed
Copy link
Member Author

Choose a reason for hiding this comment

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

Now the editor component is shown as soon as a file is selected. I've added a 200ms delay to show the loader to avoid it being shown just for a few frames when a document/editor is loaded quickly.

@@ -32,7 +32,7 @@ export type EditorProps<
A extends Action = Action,
LocalState = unknown,
> = TUiNodes & {
document: Document<T, A, LocalState>;
document: Document<T, A, LocalState> | undefined;
Copy link
Member Author

Choose a reason for hiding this comment

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

For read mode, when a document is selected for the first time, it has to be fetched before showing. This was causing the UI to stay in the Folder/file view for a while when the user clicks on a document before showing the editor.
I've changed it so that the editor supports this loading state where a file is selected but not loaded yet.
This component is probably doing too much and should be split into smaller ones.

});
}

class ReadModeContextImpl implements Omit<IReadModeContext, 'readDrives'> {
Copy link
Member Author

Choose a reason for hiding this comment

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

Tried to implement the document-drive wrapper for the readmode as class. This will be the value of the ReadModeContext.
Not very happy about this, I think a simple object will work better.

}

/* eslint-disable @typescript-eslint/no-non-null-assertion */
@checkServer
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 DocumentDriveServer has to be set when creating the context, these methods are decorated with this method that returns an error if they are called before the server is defined.

} catch (error) {
console.error(error);
}
getEnsInfo(user.address, user.chainId)
Copy link
Member Author

Choose a reason for hiding this comment

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

We no longer wait to get the ens info (user name and image) before setting the user.
The ens request sometimes takes a long time and it was delaying the login process.


const documentCacheAtom = atom(new Map<string, Document>());

const singletonFileNodeDocumentAtom = atom<FileNodeDocument>(undefined);
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 we are managing the selected document in hooks, it was causing parallel requests to be made, one for each document using the hook.
I've added these atoms which prevent that and use a single request.


// if document will be loaded then sets
// the cached version while it loads
const documentCache = get(documentCacheAtom);
Copy link
Member Author

Choose a reason for hiding this comment

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

Creates a cache for the fetched documents. When the user selects a document for the second time, he immediately sees the cached version and then the updated document when the new fetch request finishes.

const fetchDocument = useCallback(
async (driveId: string, id: string, documentType: string) => {
try {
const document = await (isReadMode
Copy link
Member Author

Choose a reason for hiding this comment

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

The users of this hook just want to get the selected document, this calls the correct method depending if it belongs to a read drive or a synced drive.

@@ -9,7 +9,7 @@ import type { User } from 'src/services/renown/types';

let userInit = false;

const userAtom = atom<User | undefined>(undefined);
const userAtom = atom<User | null | undefined>(undefined);
Copy link
Member Author

Choose a reason for hiding this comment

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

The user starts as undefined, when the initial check is finished and no user is found then it's set to null.

@acaldas
Copy link
Member Author

acaldas commented Sep 12, 2024

Looks great,

I just saw that the default editor text is not center aligned anymore. Made a small PR to fix it. I can merge it now and then you can include it in this PR.

powerhouse-inc/design-system#365

Thanks!

@acaldas acaldas merged commit ff6b7d8 into develop Sep 12, 2024
5 of 6 checks passed
@acaldas acaldas deleted the feat/read-mode branch September 12, 2024 10:48
Copy link

🎉 This PR is included in version 1.0.0-dev.105 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Copy link

🎉 This PR is included in version 1.0.0-next.11 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants