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

Notification improvements #589

Merged
merged 2 commits into from
Feb 19, 2016
Merged

Notification improvements #589

merged 2 commits into from
Feb 19, 2016

Conversation

vladjerca
Copy link
Contributor

  1. Moved position to top-right (settings icon is no longer there, so this way the toastr's do not interfere with the application in any way).
  2. I've changed the remove song toastr to a warning.
  3. I've changed the color scheme using colors from https://flatuicolors.com/, and also removed rounded edges. (the entire interface is based on strong defined corners, the toastr's just didn't belong).

@vladjerca vladjerca changed the title #588: Notification improvements Notification improvements Feb 1, 2016
@Ajeey
Copy link
Contributor

Ajeey commented Feb 1, 2016

A screenshot of previous notification and the new one would be helpful.

@Pitros
Copy link
Contributor

Pitros commented Feb 1, 2016

It's slightly better now, but still not perfect - now you can't access window action buttons on Windows. @Ajeey
Before:
image
After:
image

@@ -23,7 +23,7 @@ app.directive('favoriteSong', function(
SCapiService.deleteFavorite(userId, songId)
.then(function(status) {
if ( typeof status == "object" ) {
notificationFactory.success("Song removed from likes!");
notificationFactory.warning("Song removed from likes!");
Copy link
Contributor

Choose a reason for hiding this comment

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

There's no warning method in notificationFactory... it should be noticationFactory.warn().

@vladjerca
Copy link
Contributor Author

@Pitros Thanks for the screenshots, I was in a bit of a hurry when I made the changes, didn't take the time to take the screenshots. Hope you guys like it as much as I do.

Update 1: Further improvements could consist of slightly smaller notifications (width/height) and some padding top to be just under the main navigation bar (illusion of being in the container).

Update 2: I've made the changes, the 'warn' method is now used + I've corrected top right margins to offer a more consistent experience:
toastrtoprightfix

@Ajeey
Copy link
Contributor

Ajeey commented Feb 1, 2016

I believe if we reduce the size of the image/font-image in the notification toast, it would look more cleaner and compact.

@weblancaster
Copy link
Member

What were the changes you did to toastr.min.css and angular-toastr.min.css @vladjerca ? is it just updated to new version of them or you actually updated the code?

if you updated the code (toastr.min.css and angular-toastr.min.css) can you revert to what was before please and instead just overwrite the css on _default.scss

@vladjerca
Copy link
Contributor Author

I used the .less files provided in the official github repositories for each to make the modifications and I just compiled/minified them to replace the old ones.

I can just take the diffs and add them to _default.scss in order to override the original classes.

I'll try to tidy up both of the pull requests in the by the end of the week. (some personal matters got in the way)

@weblancaster
Copy link
Member

I think the changes are good so we just need to revert toastr.min.css and angular-toastr.min.css back to how it was and override them original on _default.scss

take your time @vladjerca

1. Moved position to top-right (settings icon is no longer there, so this way the toastr's do not interfere with the application in any way).
2. I've changed the remove song toastr to a warning.
3. I've changed the color scheme using colors from [https://flatuicolors.com/](url), and also removed rounded edges. (the entire interface is based on strong defined corners, the toastr's just didn't belong).
@vladjerca
Copy link
Contributor Author

The commit has been updated.

@weblancaster
Copy link
Member

@vladjerca thanks but when I said to override I wasn't thinking about adding important all over the place..

What you can do is something like this.. override by selector hierarchy

#app { // or body or html instead of #app
// here the toast style without important e.g below
.toast-message a {}
}

@vladjerca
Copy link
Contributor Author

@weblancaster Yeah, my bad on that one. I've updated the scss file.

@weblancaster
Copy link
Member

@vladjerca below is how looks like now (not as your screenshot).. what you want to do is use body instead of #app that's why I put the comment to use the selector that would add the hierarchy.

you can also move the code below to be inside with the other selectors

.toast-message a,
    .toast-message label {
        font-size: 0.900em;
        color: #ecf0f1;
    }

    .toast-message a:hover {
        color: #bdc3c7;
    }

screen shot 2016-02-04 at 11 11 09 am

@vladjerca
Copy link
Contributor Author

@weblancaster Updated, sorry for all the mishaps.

weblancaster added a commit that referenced this pull request Feb 19, 2016
@weblancaster weblancaster merged commit a0b54bf into Soundnode:master Feb 19, 2016
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.

4 participants