-
Notifications
You must be signed in to change notification settings - Fork 136
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
Three column layout (rebased/squashed) #1021
Conversation
9b18098
to
e117cc1
Compare
605fa47
to
d5accf8
Compare
d5accf8
to
f6364d7
Compare
977523e
to
003c804
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will test the function tomorrow, but here some comments on the code
}, | ||
|
||
displayedNotes() { | ||
if (this.filteredNotes.length > 40 && this.showFirstNotesOnly) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why you catch for > 40
but slice to 0-30
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something to consider separately I'd say as it is not introduced here but originating from https://github.com/nextcloud/notes/pull/424/files#diff-84c733529b3b3fd6edfc369f85fe95b5cfb8d1f0a74d39712293aa3b037a0848R113-R117 s
if (note.favorite) { | ||
return '' | ||
} | ||
const t = note.modified * 1000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May move 1000
to const variable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd still keep this as this is a pattern that js developers likely know well and a constant (or timestampToMilliseconds helper that I was thinking about) would be easy to be overseen in new code introduced and then we'd have two ways of doing that.
Thanks for the review @jonnytischbein, very much appreciated. I addressed most of them, two I'd keep out of this PR. |
8bb2d35
to
c2fed2b
Compare
Signed-off-by: Joachim von Eichborn <[email protected]>
Signed-off-by: Julius Härtl <[email protected]>
… router-link Signed-off-by: Julius Härtl <[email protected]>
Signed-off-by: Julius Härtl <[email protected]>
Signed-off-by: Julius Härtl <[email protected]>
Signed-off-by: Julius Härtl <[email protected]>
Signed-off-by: Julius Härtl <[email protected]>
Signed-off-by: Julius Härtl <[email protected]>
Signed-off-by: Julius Härtl <[email protected]>
Signed-off-by: Julius Härtl <[email protected]>
c2fed2b
to
6e03368
Compare
Let's get this in so we can have a first beta for more testing. |
Hey,
Do I have to do any migration steps ? |
Hm, I cannot think of any. Can you test with a new user account to see if it might be related to the account settings with which editor is being used as I see NotePlain in the trace? |
Rebased and squashed version of #842 to have an easier way to resolve the conflicts
So far pushed another commit additionally to bring back the actions menu in the list after my rebase and some minor styling fixes