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

implemented dynamic weather based on current location (IP) #16

Closed
wants to merge 28 commits into from

Conversation

kaykouo
Copy link

@kaykouo kaykouo commented Aug 30, 2022

added dynamic weather based on current location (IP)
added sunrise and sunset time of current location
added hint for user to press spacebar for searching
replaced font by ubuntu

Copy link
Owner

@Jaredk3nt Jaredk3nt left a comment

Choose a reason for hiding this comment

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

Most of these changes seem like things you wanted in your own fork of the project and do not belong in the original. I'm glad to see people forking & experimenting with this but it's always best to open an issue first before just opening a PR with these kind of changes.

Also always attach screenshots of visual changes. I would rather not have to pull down your branch to check things out.

},
{ name: "/r/unixporn", url: "https://reddit.com/r/unixporn" },
{ name: "/r/news", url: "https://reddit.com/r/news" },
{ name: "TikTok", url: "https://tiktok" },
Copy link
Owner

Choose a reason for hiding this comment

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

Please don't make changes to the urls like here and in the README. These are fine in your fork but not in the primary repository

}

// Display random jokes
function getRandomJokes(){
Copy link
Owner

Choose a reason for hiding this comment

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

Please don't add completely new functionality like this to the main repo without opening an issue first asking if it's okay. I don't think jokes fit with this projects intent

@@ -1,4 +1,5 @@
@import url('https://fonts.googleapis.com/css?family=Roboto+Mono');
/* @import url('https://fonts.googleapis.com/css?family=Roboto+Mono'); */
@import url('https://fonts.googleapis.com/css?family=Ubuntu:regular,bold&subset=Latin');
Copy link
Owner

Choose a reason for hiding this comment

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

Please don't modify the design of the original project without first opening an issue and asking about the changes first. The project was designed to use Roboto.

@kaykouo
Copy link
Author

kaykouo commented Sep 2, 2022

please forgive the newbie here 🤣 I didn't meant to merge so much. At the beginning I just wanted to add the dynamic weather api. but then I forgot to switch to my private repo, in which I changed a lot like adding dynamic public holidays api etc.. you could take a look here if you want: https://kayou.tk .I will close this request. You know what I mean, I'd be more than happy to help you get some new idea.

@kaykouo kaykouo closed this Sep 2, 2022
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

2 participants