Looking for a new feature branch for exporting! #3
Conversation
pranitsh
changed the title
| cy.get('.ce-paragraph').eq(3).type("4{enter}"); | ||
| cy.get('.ce-paragraph').eq(4).type("5"); | ||
| cy.get('.ce-header').click().type("Title{enter}"); | ||
| cy.get('.ce-paragraph').type("1{enter}2{enter}3{enter}4{enter}5"); |
There was a problem hiding this comment.
Okay, it makes sense and is more concise, but it requires an assertion.
There was a problem hiding this comment.
Which assertion do you suggest?
| return doc._data | ||
| }) | ||
| if (doc._data) { | ||
| const propertiesToDelete = [ |
There was a problem hiding this comment.
The variable should be declared outside of the loops.
There was a problem hiding this comment.
Right, any suggested places for this constant?
| @@ -385,12 +370,10 @@ export class Shell { | |||
| this.editor.blocks.update(block.id, block.data); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Why did you change this to async if you don't require await?
There was a problem hiding this comment.
Sorry, kinda struggling to figure out what this is referencing. Could you please clarify which async?
| const blocks = JSON.parse(await input.files[0].text()).blocks; | ||
| await this.manager.bulkInsertBlocks(blocks); | ||
| const currentBlocks = (await this.manager.exportNotebook(url.read("p"), url.read("t"))).blocks; | ||
| let currentMaxndex = currentBlocks.reduce((max, _, index) => Math.max(max, index), 0); |
There was a problem hiding this comment.
It is unnecessary because you can use:
currentMaxIndex = currentBlocks.lenght-1;
Blocks have their own index because they are stored randomly in the database, but they are presented in order to the user. So, if you want to store new blocks after the current ones, you only need the number of blocks.
There was a problem hiding this comment.
It's hard exactly to remember what the rationale behind this was, but I believe somehow the numbers didn't match, and I wanted to use something different and was experimenting with getting the correct numbers for the place to insert. Seems I will have to change my approach either way.
| currentMaxndex += 1; | ||
| } | ||
| return block; | ||
| }).filter((block: BlockDocument | undefined) => block !== undefined).forEach; |
There was a problem hiding this comment.
Why did you do with the forEach?
There was a problem hiding this comment.
Sorry, some of this is just me getting my thoughts together. Seems I missed that!
| const data = await this._database?.exportJSON(); | ||
| let toExport: Array<BlockDocument> = []; | ||
| data?.collections[1].docs.forEach((element) => { | ||
| if (element.createdBy === peer) { | ||
| if (element.createdBy === peer && element.topic === topic) { |
There was a problem hiding this comment.
The topic is an identifier for a set of blocks intended to represent a notebook, so it should be
if (element.topic === topic)
There was a problem hiding this comment.
Thank you for the clarification!
|
I've provided you with the develop branch. Please use it. Additionally, every time you make a pull request, Firebase creates a preview website to showcase your progress. https://github.com/sanchezcarlosjr/evanotebook/blob/main/.github/workflows/firebase-hosting-pull-request.yml |
|
Thank you for reviewing my pull request! I appreciate the feedback! |
Oh, I didn't know, following the GitHub workflow is a bit difficult, and I kind of don't know how Firebase hosting works, since I haven't needed such a service before. I'll look into it a bit in the future, though could you clarify where this website link is if possible? I'll also use the new |
|
Oh, found it: https://github.com/sanchezcarlosjr/evanotebook/actions/runs/7946059563 I believe I heard PRs can still allow commits. You should see new commits with some of the comments resolved by Wednesday (after some hw is due), though I would really appreciate addressing this after this discussion. All changes follow the current license you have. Thanks again for providing me with feedback. This is essentially my first pull request and I'm very appreciative of you taking the time to review it. |
A lot of the changes I wish to make regard the database, so I'm looking for a new branch (probably not the main branch, except for some of the earlier commits) to test my features and get some more guidance on what works and what I should change. This is my first pull request, so I'm more looking for your thoughts rather than merging at the moment, especially for how this approach looks.