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.

Sign up for GitHub

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

Jump to bottom

Improve alert list dialog links #87

Merged
merged 1 commit into from
Mar 8, 2018
Merged

Improve alert list dialog links #87

merged 1 commit into from
Mar 8, 2018

Conversation

psiinon
Copy link
Member

@psiinon psiinon commented Mar 5, 2018

Currently the alert list dialogs show:

  • Alert name
    • Alert name (index)
    • Alert name (index)
    • Alert name (index)

This isnt that useful :)
Changed to be similar to the desktop UI:

  • Alert name (count)
    • URL 1
    • URL 2
    • URL 3

@psiinon
Copy link
Member Author

psiinon commented Mar 5, 2018

Its worth noting that I had to turn CSP off in order to test this ;) Re #85

@thc202
Copy link
Member

thc202 commented Mar 5, 2018

Could the ID be removed from the name when showing the alert details? (e.g. Test Alert (22))

@psiinon
Copy link
Member Author

psiinon commented Mar 5, 2018

Good suggestion - done :)

@thc202
Copy link
Member

thc202 commented Mar 5, 2018

Thanks!

@dscrobonia
Copy link
Contributor

Much better! Thanks for the changes Simon!

dscrobonia
dscrobonia approved these changes Mar 8, 2018
View reviewed changes
Copy link
Contributor

@dscrobonia dscrobonia left a comment

Choose a reason for hiding this comment

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

lgtm

@kingthorin kingthorin merged commit 1e8fff9 into master Mar 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thc202 thc202 thc202 approved these changes

@dscrobonia dscrobonia dscrobonia approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@psiinon @thc202 @dscrobonia @kingthorin