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

Added ScreenSaver for sugarizer #46

Closed
wants to merge 7 commits into from
Closed

Added ScreenSaver for sugarizer #46

wants to merge 7 commits into from

Conversation

crusher95
Copy link

No description provided.

@davelab6
Copy link
Contributor

davelab6 commented Apr 2, 2016

@crusher95 please rebase your branch and fixup the 2nd commit into a single commit :)

https://www.google.com/search?q=git+rebase+interactive

@davelab6
Copy link
Contributor

davelab6 commented Apr 2, 2016

What is the license for js/screensaver.jpg? I think this is an image subject to restrictive copyrights from threadless.com

@davelab6
Copy link
Contributor

davelab6 commented Apr 2, 2016

Why is css/styles.css mod from 100644 → 100755?

The only changes to css/styles.css relevant to this PR seem to be

#screen{
    z-index: 100000;
}

The code-style can be improved (adding a space before {)

@llaske do you have a code style linter?

@@ -1112,4 +1107,4 @@ input {
.journal-filter-time {
visibility: visible;
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It is good practice to have a final blank newline at the end of every text line to prevent concatenated output merging the last and first lines of files.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a blank new line to the end of the file

@@ -126,7 +126,6 @@ input {
.search-field-input {
opacity: 1;
margin-left: 30px;
margin-top: 5px;
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this changed?

@davelab6
Copy link
Contributor

davelab6 commented Apr 3, 2016

965a5f8 adds a new gif, but what is the license for this? Where is it from?

@llaske
Copy link
Owner

llaske commented Apr 3, 2016

Don't work here. image is null line 12 of the screensaver.js script both on Chrome and Firefox.
Plus I'm not at all a big fan of the idea of a screen saver. It has no sense on devices where screen saving is already handle (tablet, smartphone and PC) and I don't see the interest of a screen saver in a web browser.

@crusher95
Copy link
Author

Hi,

Yes point accepted (y) . Is there any other issue that you could assign me
or suggest me for sugarizer?

Thanking You,
Utkarsh Dhawan
(Developer/Student)

Note:Please Consider The environment before printing.

On Sun, Apr 3, 2016 at 2:16 PM, Lionel LASKE notifications@github.com
wrote:

Don't work here. image is null line 12 of the screensaver.js script both
on Chrome and Firefox.
Plus I'm not at all a big fan of the idea of a screen saver. It has no
sense on devices where screen saving is already handle (tablet, smartphone
and PC) and I don't see the interest of a screen saver in a web browser.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#46 (comment)

@davelab6
Copy link
Contributor

davelab6 commented Apr 3, 2016 via email

@llaske
Copy link
Owner

llaske commented Apr 4, 2016

Hi Utkarsh, I don't have any issue to assign, except if you find one in testing.
BTW, if you've got some time, may be you could try to port some existing activities. Why not FractionBounce ? http://activities.sugarlabs.org/en/sugar/addon/4488

@llaske llaske closed this Apr 4, 2016
@crusher95
Copy link
Author

Hi,

An interesting activity indeed. I am on it (y)

Thanking You,
Utkarsh Dhawan
(Developer/Student)

Note:Please Consider The environment before printing.

On Tue, Apr 5, 2016 at 1:23 AM, Lionel LASKE notifications@github.com
wrote:

Closed #46 #46.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#46 (comment)

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.

None yet

3 participants