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

Clear log #180

Merged
merged 6 commits into from Aug 6, 2019

Conversation

@UziTech
Copy link
Contributor

commented Dec 11, 2017

Description of the Change

This adds a clear notifications button to the log

image

Alternate Designs

At the moment the button clears the notifications with atom.notifications.clear() as well as clearing the log and any displayed notifications.

I would like to get it working with atom.notifications.onDidClearNotifications() once that gets released.

Benefits

Ability to clear the notifications log

Possible Drawbacks

none

Applicable Issues

#164 (comment)

@UziTech UziTech force-pushed the UziTech:clear-log branch from f7198f8 to a63b064 Dec 11, 2017

@50Wliu

This comment has been minimized.

Copy link
Member

commented Dec 11, 2017

Hmm. I can't say I'm the biggest fan of the dash meaning "clear", but I also don't see any better octicon. Have you considered maybe just a button that says "Clear All" (or some variant)?

@UziTech

This comment has been minimized.

Copy link
Contributor Author

commented Dec 11, 2017

I did have the button say clear initially. The problem I saw was when the Logs dock is on the right the button took up too much real estate for how important it is. It would be nice to find an icon that would be intuitive.

@50Wliu

This comment has been minimized.

Copy link
Member

commented Jan 3, 2018

@simurai do you have any thoughts about this? On my screen there's definitely enough space for text when in a side dock.
notifications-clear-all-space

Maybe the Clear button could wrap to a new line if there isn't enough room so that we can keep it as text.

@simurai

This comment has been minimized.

Copy link
Member

commented Jan 3, 2018

Maybe trashcan?

image

Like "deleting" all notifications.

@simurai

This comment has been minimized.

Copy link
Member

commented Jan 3, 2018

Or circle-slash?

screen shot 2018-01-03 at 3 09 40 pm

DevTools uses that to clear the console. Although we have been using it for the "forbidden/not allowed" cursor.

@UziTech

This comment has been minimized.

Copy link
Contributor Author

commented Jan 3, 2018

I like the trashcan. That seems to be the most intuitive.

@subscriptions.add atom.commands.add 'atom-workspace', 'notifications:clear-log', =>
for notification in atom.notifications.getNotifications()
notification.options.dismissable = true
notification.dismissed = false

This comment has been minimized.

Copy link
@50Wliu

50Wliu Feb 4, 2018

Member

Why is this needed? Under what circumstances will there be an already-dismissed notification in the list that we need to dismiss again?

This comment has been minimized.

Copy link
@UziTech

UziTech Feb 4, 2018

Author Contributor

non-dimissable notifications (ones that dismiss automatically) have .dismissed = true when they are first shown. If someone clears the log we want those notifications to be dismissed right away not wait for the timer to dismiss them automatically.

@UziTech UziTech referenced this pull request Feb 5, 2018
@UziTech

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

Can this get reviewed?

@jasonrudolph
Copy link
Member

left a comment

👋 @UziTech: I've reviewed this pull request and noted a few questions below. I hope this helps.

lib/main.coffee Outdated Show resolved Hide resolved
lib/notifications-log.coffee Outdated Show resolved Hide resolved

.notifications-clear-log {
float: right;
}

This comment has been minimized.

Copy link
@jasonrudolph

jasonrudolph Aug 5, 2019

Member

@UziTech: When you have a moment, can you share screenshots of this new icon is rendered in each of the UI themes that ship with Atom? I suspect it renders just fine, but we want to make sure. 😅

This comment has been minimized.

Copy link
@UziTech

UziTech Aug 5, 2019

Author Contributor

Atom Dark

image

Atom Light

image

One Dark

image

One Light

image

@UziTech UziTech force-pushed the UziTech:clear-log branch from fdb1ed2 to 605cc82 Aug 5, 2019

@UziTech

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

I rebased and fixed the issues

@jasonrudolph

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

I rebased and fixed the issues

Cool! Thanks for your perseverance on this pull request, @UziTech. ⚡️

@jasonrudolph jasonrudolph merged commit df6e81d into atom:master Aug 6, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.