Skip to content

Conversation

@D-Bao
Copy link
Contributor

@D-Bao D-Bao commented May 5, 2023

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.

Copy link
Contributor

@nguyenkims nguyenkims left a 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
Copy link
Contributor

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;
Copy link
Contributor

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: "",
Copy link
Contributor

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;
Copy link
Contributor

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 ]].
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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() {
Copy link
Contributor

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],
Copy link
Contributor

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?

Copy link
Contributor

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);
Copy link
Contributor

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()

Copy link
Contributor

@nguyenkims nguyenkims left a 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.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants