search

pekrau / BeerClub

A web application to keep track of the beer accounts for registered members.
3 stars 2 forks source link

Readme with dev instructions. New logo with dark background for nav #96

Closed ewels closed 5 years ago

ewels commented 5 years ago

Hi @pekrau,

I managed to get it running locally! I wrote in the readme the steps I took to get there. I also had to make a few changes to the code:

  1. I had to move app_beerclub.py up one directory to the repo root to get the from beerclub imports to work
  2. The settings file seemed to be missing variables for the database, so I added these to the template
  3. The template JSON file was added to the repo before the .gitignore ignored all JSON files. However, because it was covered by .gitignore, my editor didn't show it (which confused me for a while!). I explicitly allowed that one file in .gitignore to get around this.

The readme instructions are then just the steps that I took to get it running. Feel free to correct / change as necessary. It includes stuff like me figuring out the required Python version based on the imports and other things, which may be imprecise.

Finally - the reason for doing all of this, I hopefully resolved #95 by making an SVG version of the logo in light and dark modes. The dark version has white text and is much more clear. It's still relatively small, but hopefully it's ok:

image

Phil

ewels commented 5 years ago

I realised that the deployed SciLifeLab beer club has a different colour for the navbar. So now there's an option for the logo - I recommend:

"DISPLAY_NAVBAR_LOGO": "BeerClubLogo.svg",

This then gives:

image

Not ideal as there's green text on a green background though...

ewels commented 5 years ago

ok ok, use "DISPLAY_NAVBAR_LOGO": "BeerClubLogo_MonoSciLifeLab.svg", and I think we're good:

image

I also made the favicon have a transparent background:

image

ewels commented 5 years ago

Should be good to merge now I think if you're happy 😅

pekrau commented 5 years ago

Hi Phil, I'm at a conference today and tomorrow, will look at it after that. In general, sounds very good. Install instructions badly needed. I am not happy with the move of app.py. The problem you have should be dealt with by adding the top level dir to the python path. /Per K

Den ons 22 maj 2019 15:15Phil Ewels notifications@github.com skrev:

Hi @pekrau https://github.com/pekrau,

I managed to get it running locally! I wrote in the readme the steps I took to get there. I also had to make a few changes to the code:

  1. I had to move app_beerclub.py up one directory to the repo root to get the from beerclub imports to work
  2. The settings file seemed to be missing variables for the database, so I added these to the template
  3. The template JSON file was added to the repo before the .gitignore ignored all JSON files. However, because it was covered by .gitignore, my editor didn't show it (which confused me for a while!). I explicitly allowed that one file in .gitignore to get around this.

The readme instructions are then just the steps that I took to get it running. Feel free to correct / change as necessary. It includes stuff like me figuring out the required Python version based on the imports and other things, which may be imprecise.

Finally - the reason for doing all of this, I hopefully resolved #95 https://github.com/pekrau/BeerClub/issues/95 by making an SVG version of the logo in light and dark modes. The dark version has white text and is much more clear. It's still relatively small, but hopefully it's ok:

[image: image] https://user-images.githubusercontent.com/465550/58175098-aae09500-7c9f-11e9-858e-5cbad4b09ba6.png

Phil

You can view, comment on, or merge this pull request online at:

https://github.com/pekrau/BeerClub/pull/96 Commit Summary

  • Readme with dev instructions. New logo with dark background for nav
  • More settings options for the navbar logo

File Changes

Patch Links:

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/pekrau/BeerClub/pull/96?email_source=notifications&email_token=AADGJXBD5VSZ5NBE6SLBOGLPWVBP5A5CNFSM4HOUF54KYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4GVGVJMA, or mute the thread https://github.com/notifications/unsubscribe-auth/AADGJXDGAADA67IK6DP75ULPWVBP5ANCNFSM4HOUF54A .

pekrau commented 5 years ago

I don't see that you have actually moved the app_beerclub.py file up one level in this PR? Am I missing something?

Anyhow, I do not want to do that. It goes against what Roman taught me when I started using GitHub, that all code should be one level down from the top level. And it will be contrary do how I do things in every other repo. So if that move up one level is there, I will not accept the PR, even though everything else seems perfectly fine.

I hope I do not sound ungrateful. I appreciate everything you do. It's just that in this particular instance, I am using my "benevolent dictator for life" prerogative regarding BeerClub.

ewels commented 5 years ago

Sure, it's here: https://github.com/pekrau/BeerClub/pull/96/files?file-filters%5B%5D=.py#diff-ff5d928bd92013f564eed5e8911dc6bc

No problem to not move it, but I dislike having to manually add stuff to the (python-) PATH. In my mind that should either that should be done by installing as a package using a setup.py or better still, using relative paths (as Python can by default).

An easy fix is to do from . import xxx instead of from beerclub import xxx, then the file doesn't need to be moved. I'll do this now.

Phil

ewels commented 5 years ago

ps. You've actually already done this in places, eg. here: https://github.com/pekrau/BeerClub/blob/139700eb4d4e7c9dab855ce2ef33a2276a8b469a/beerclub/event.py#L9-L11

ewels commented 5 years ago

pps. Also, not quite every other repo - eg. Genomics Status has the same layout: https://github.com/SciLifeLab/genomics-status/blob/master/status_app.py (that's the only other Tornado website I've worked on I think - Flask sites have a very specific code structure usually with a weird way of launching them with a generic cli).

ewels commented 5 years ago

Ok, the relative import thing didn't work easily when it wasn't installed as a module, so I just updated the README.md to include description of the PYTHONPATH instead 👍

pekrau commented 5 years ago

Yes, I was just going to point out that problem. I am at a conference today, will look at it tomorrow. Of course, a bug has appeared in the OrderPortal, so that will have priority. /Per K

Den tor 23 maj 2019 11:51Phil Ewels notifications@github.com skrev:

Ok, the relative import thing didn't work easily when it wasn't installed as a module, so I just updated the README.md to include description of the PYTHONPATH instead 👍

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/pekrau/BeerClub/pull/96?email_source=notifications&email_token=AADGJXDUHRQZP2P4N4LZ3Y3PWZSIJA5CNFSM4HOUF54KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWBWDSI#issuecomment-495149513, or mute the thread https://github.com/notifications/unsubscribe-auth/AADGJXGPQUQUT7NYX74FDQTPWZSIJANCNFSM4HOUF54A .

pekrau commented 5 years ago

I haven't accepted your PR yet, will do on Monday. I do not understand your resetting problem at all. Will look at Monday. /Per K

Den tors 23 maj 2019 kl 12:41 skrev Per Kraulis per.kraulis@scilifelab.se:

Yes, I was just going to point out that problem. I am at a conference today, will look at it tomorrow. Of course, a bug has appeared in the OrderPortal, so that will have priority. /Per K

Den tor 23 maj 2019 11:51Phil Ewels notifications@github.com skrev:

Ok, the relative import thing didn't work easily when it wasn't installed as a module, so I just updated the README.md to include description of the PYTHONPATH instead 👍

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/pekrau/BeerClub/pull/96?email_source=notifications&email_token=AADGJXDUHRQZP2P4N4LZ3Y3PWZSIJA5CNFSM4HOUF54KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWBWDSI#issuecomment-495149513, or mute the thread https://github.com/notifications/unsubscribe-auth/AADGJXGPQUQUT7NYX74FDQTPWZSIJANCNFSM4HOUF54A .

-- Per Kraulis, Ph.D. Data Engineer, Data Centre, SciLifeLab. Dept Biochemistry and Biophysics, Stockholm University. +46 (0)70 639 9635 http://www.scilifelab.se/ Visiting address: Tomtebodavägen 23A, Gamma, floor 5, Karolinska Institutet Science Park, Solna Postal address: SciLifeLab Stockholm, Box 1031, 171 21 Solna, Sweden

pekrau commented 5 years ago

I have tried the reset using my test account and it works.

Exactly what is you do, and exactly what is the error message?

/Per K

Den lör 25 maj 2019 kl 22:23 skrev Per Kraulis per.kraulis@scilifelab.se:

I haven't accepted your PR yet, will do on Monday. I do not understand your resetting problem at all. Will look at Monday. /Per K

Den tors 23 maj 2019 kl 12:41 skrev Per Kraulis <per.kraulis@scilifelab.se

:

Yes, I was just going to point out that problem. I am at a conference today, will look at it tomorrow. Of course, a bug has appeared in the OrderPortal, so that will have priority. /Per K

Den tor 23 maj 2019 11:51Phil Ewels notifications@github.com skrev:

Ok, the relative import thing didn't work easily when it wasn't installed as a module, so I just updated the README.md to include description of the PYTHONPATH instead 👍

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/pekrau/BeerClub/pull/96?email_source=notifications&email_token=AADGJXDUHRQZP2P4N4LZ3Y3PWZSIJA5CNFSM4HOUF54KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWBWDSI#issuecomment-495149513, or mute the thread https://github.com/notifications/unsubscribe-auth/AADGJXGPQUQUT7NYX74FDQTPWZSIJANCNFSM4HOUF54A .

-- Per Kraulis, Ph.D. Data Engineer, Data Centre, SciLifeLab. Dept Biochemistry and Biophysics, Stockholm University. +46 (0)70 639 9635 http://www.scilifelab.se/ Visiting address: Tomtebodavägen 23A, Gamma, floor 5, Karolinska Institutet Science Park, Solna Postal address: SciLifeLab Stockholm, Box 1031, 171 21 Solna, Sweden

-- Per Kraulis, Ph.D. Data Engineer, Data Centre, SciLifeLab. Dept Biochemistry and Biophysics, Stockholm University. +46 (0)70 639 9635 http://www.scilifelab.se/ Visiting address: Tomtebodavägen 23A, Gamma, floor 5, Karolinska Institutet Science Park, Solna Postal address: SciLifeLab Stockholm, Box 1031, 171 21 Solna, Sweden

ewels commented 5 years ago

The reset worked for me earlier too, not sure what I was doing, sorry. Ignore me!

ewels commented 5 years ago

🎉

ewels commented 5 years ago

Thanks @pekrau - looking good!

Reminder that you can set "DISPLAY_NAVBAR_LOGO": "BeerClubLogo_MonoSciLifeLab.svg" if you want, to avoid the green on green text.. (https://github.com/pekrau/BeerClub/pull/96#issuecomment-494800679)

pekrau commented 5 years ago

Aha, sorry, forgot that after having set the initial value in init.settings.

done.

Den mån 27 maj 2019 kl 13:10 skrev Phil Ewels notifications@github.com:

Thanks @pekrau https://github.com/pekrau - looking good!

Reminder that you can set "DISPLAY_NAVBAR_LOGO": "BeerClubLogo_MonoSciLifeLab.svg" if you want, to avoid the green on green text.. (#96 (comment) https://github.com/pekrau/BeerClub/pull/96#issuecomment-494800679)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/pekrau/BeerClub/pull/96?email_source=notifications&email_token=AADGJXAR5PB7XFRMQ7S3ZWTPXO6RZA5CNFSM4HOUF54KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWJQVQI#issuecomment-496175809, or mute the thread https://github.com/notifications/unsubscribe-auth/AADGJXDNF36ME4YBQG2U3J3PXO6RZANCNFSM4HOUF54A .

-- Per Kraulis, Ph.D. Data Engineer, Data Centre, SciLifeLab. Dept Biochemistry and Biophysics, Stockholm University. +46 (0)70 639 9635 http://www.scilifelab.se/ Visiting address: Tomtebodavägen 23A, Gamma, floor 5, Karolinska Institutet Science Park, Solna Postal address: SciLifeLab Stockholm, Box 1031, 171 21 Solna, Sweden

ewels commented 5 years ago

Looks awesome!