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

MailboxPicker #3571

Merged
merged 1 commit into from
Sep 12, 2020
Merged

Conversation

dehnhardt
Copy link
Collaborator

@ChristophWurst and @st3iny: I split the mailbox picker out of the big PR and incorporated Christoph's comments.
For viewing and testing I simply added a mailbox picker to the account settings. Before the merge we remove this ;-)

src/components/MailboxPicker.vue Outdated Show resolved Hide resolved
src/components/MailboxPicker.vue Outdated Show resolved Hide resolved
},
methods: {
getMailboxes(root, folderid) {
let folders = []
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let folders = []
let mailboxes = []

for consistency :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We now need 2 arrays here...

Copy link
Member

Choose a reason for hiding this comment

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

Not necessarily. Instead of iterating on one array and filling another one (imperative coding) you can map the mailboxes to the new data structure and just return the result. No need to store anything a second time. If that's too much to ask then leave it and I can clean up the code later ✌️

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's definitly not too much to ask, I simply do not understand how to do that. Can you give me a hint?

Copy link
Member

Choose a reason for hiding this comment

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

Then I'm sorry for not explaining it.

Currently you do

let mailboxes = ...
const boxLevel = []
mailboxes.forEach((folder) => {
    ...
    boxLevel.push({ ... })
})
return boxLevel

but you can simplify to

let mailboxes = ...
return mailboxes.map((mailbox) => {
    return {
        ...,
        children: this.getMailboxes(mailbox.databseId)
    }
})

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Wow, strange syntax ;-) Done!

src/components/MailboxPicker.vue Outdated Show resolved Hide resolved
src/views/AccountSettings.vue Outdated Show resolved Hide resolved
src/components/MailboxPicker.vue Outdated Show resolved Hide resolved
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Wörks nicely!

I guess we can improve the styling later on with icons and so on, but it's a great start. Thanks a lot!

@ChristophWurst
Copy link
Member

Please remove the component from the settings page again, squash and rebase your comments then I'll integrate :)

Signed-off-by: Holger Dehnhardt <holger@dehnhardt.org>
@dehnhardt dehnhardt force-pushed the feature/MailboxPicker branch from f6c074b to afba6d4 Compare September 12, 2020 06:49
@dehnhardt
Copy link
Collaborator Author

Please remove the component from the settings page again, squash and rebase your comments then I'll integrate :)

Done.

@ChristophWurst ChristophWurst marked this pull request as ready for review September 12, 2020 08:14
@ChristophWurst ChristophWurst merged commit 5196187 into nextcloud:master Sep 12, 2020
@ChristophWurst ChristophWurst mentioned this pull request Oct 2, 2020
2 tasks
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