-
-
Notifications
You must be signed in to change notification settings - Fork 440
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
(WIP) new Aliases SPA using Vue.js #1719
base: master
Are you sure you want to change the base?
Conversation
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.
Hey the new page seems promising 👍. I just added some comments, some don't have to be done in this PR. And can we try to have the same layout as the current one? Apart from the filter that we can handle later, I notice some differences:
- there's no "more" button in alias card
- pin should be a toggle and not a button
- when an item is pinned, a pin icon is added next to its address and the pin toggle is on.
- the transfer button doesn't work: we can just send user to the transfer page as it's the case currently.
|
||
// variables for aliases list | ||
isFetchingAlias: true, | ||
aliasesArray: [], // array of existing alias |
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.
can we rename this to just aliases
for short?
if (res.ok) { | ||
const aliasOptions = await res.json(); | ||
this.aliasSuffixes = aliasOptions.suffixes; | ||
this.aliasSelectedSignedSuffix = this.aliasSuffixes[0].signed_suffix; |
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.
as this is expired after a short time, I think it's better to only load alias options when user clicks on the "new custom alias" button.
aliasesArrayOfNextPage: [], // to know there is a next page if not empty | ||
page: 0, | ||
isLoadingMoreAliases: false, | ||
searchString: "", |
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 think we can name this query
:)
}, | ||
|
||
async fetchAlias(page, query) { | ||
this.isFetchingAlias = true; |
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.
should we show a loading indicator when isFetchingAlias = true?
<i class="fa fa-ban mr-2"></i> Blocked. | ||
</span> | ||
<!-- TODO improve date formatting --> | ||
Created [[ alias.creation_date ]]. |
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 think we can use this code snippet https://stackoverflow.com/a/6109105 here to have a relative date time.
this.aliasesArray.unshift(alias); | ||
toastr.success(`${alias.email} is pinned`); | ||
} else { | ||
// unpin: make alias appear at the bottom of the alias list |
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.
Though moving a pinned alias to the top is a good idea, handling the reverse case when an alias is unpinned is much harder. I think let's just not handle this for now and leave the item where it is :)
// variables for aliases list | ||
isFetchingAlias: true, | ||
aliasesArray: [], // array of existing alias | ||
aliasesArrayOfNextPage: [], // to know there is a next page if not empty |
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 think this can be renamed to nextPageAliases :)
this.isLoadingMoreAliases = false; | ||
}, | ||
|
||
resetFilter() { |
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.
We need to empty the aliases and load aliases again when resetting filter.
body: JSON.stringify({ | ||
alias_prefix: this.aliasPrefixInput, | ||
signed_suffix: this.aliasSelectedSignedSuffix, | ||
mailbox_ids: [this.defaultMailboxId], |
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.
should we allow user to select multiple mailboxes in the create alias modal?
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.
we can also just add a TODO to not forget :)
|
||
if (res.ok) { | ||
const alias = await res.json(); | ||
this.aliasesArray.unshift(alias); |
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.
we can also scroll to this new alias as user can be way below the viewport by using scrollIntoView()
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.
Hey the new page seems promising 👍. I just added some comments, some don't have to be done in this PR. And can we try to have the same layout as the current one? Apart from the filter that we can handle later, I notice some differences:
- there's no "more" button in alias card
- pin should be a toggle and not a button
- when an item is pinned, a pin icon is added next to its address and the pin toggle is on.
- the transfer button doesn't work: we can just send user to the transfer page as it's the case currently.
This PR creates a new Aliases page which uses the latest API endpoints to do operations on aliases such as updating, deleting, disabling, creating, searching etc... without having to refresh the page like the current Aliases page does.