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

[MRG] Ovh logo #286

Merged
merged 17 commits into from
Dec 4, 2019
Merged

[MRG] Ovh logo #286

merged 17 commits into from
Dec 4, 2019

Conversation

maikia
Copy link
Contributor

@maikia maikia commented Nov 20, 2019

Reference Issue

What does this implement/fix? Explain your changes.

Added Ovh logo to the front webpage and removed other powered-by logos

Any other comments?

@codecov
Copy link

codecov bot commented Nov 20, 2019

Codecov Report

Merging #286 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #286      +/-   ##
==========================================
+ Coverage   92.23%   92.24%   +<.01%     
==========================================
  Files          94       94              
  Lines        6904     6908       +4     
==========================================
+ Hits         6368     6372       +4     
  Misses        536      536
Impacted Files Coverage Δ
ramp-frontend/ramp_frontend/views/general.py 96.15% <100%> (+0.69%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ba84e2...70e4621. Read the comment docs.

Copy link
Collaborator

@agramfort agramfort left a comment

Choose a reason for hiding this comment

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

ok for you @kegl for the new server?

@kegl
Copy link
Collaborator

kegl commented Nov 20, 2019

We need to find a way to customize some of the web pages (landing, sign up). Logos is one thing, but for example we needed to add terms and conditions to the sign up page. I included it in the template for now as plain html, but I will not commit it in the master. Since these may come up later, what is important is to find a way to do this seamlessly.

For example, have placeholders and defaults for those committed in the master, and a config template folder in the deployment where customizable elements can be changed. Then we can copy those into the template directory (ignored by github) when for example the frontend is launched.

See also this comment: #276 (comment)

@agramfort
Copy link
Collaborator

agramfort commented Nov 21, 2019 via email

@maikia
Copy link
Contributor Author

maikia commented Nov 22, 2019

@maikia can you see if it's possible to make it a configurable thing?

@kegl @agramfort
Sure, I do my best. Just to be clear as I never worked with placeholders and defaults (or html for that matter ;) )

  1. you want the frontpage to be easily customizable (logos, pictures, terms and conditions, sections, etc) before the launch?
  2. How about after the launch? If like now we need to change logo(s) (or people, or other info) on the running RAMP... Is there a way and if yes is it of interest to have possibility of making some changes like that to already launched RAMP?
  3. finally, how much of a priority is it?

@agramfort
Copy link
Collaborator

agramfort commented Nov 22, 2019 via email

@kegl
Copy link
Collaborator

kegl commented Nov 22, 2019

Ideally we should be able to change it after the launch. I simply edited the sign up template https://github.com/paris-saclay-cds/ramp-board/blob/master/ramp-frontend/ramp_frontend/templates/sign_up.html adding pure html to it. You can do the same thing temporarily at your end to add logos. The problem is that sign_up.html is in the repo and I don't want to commit Huawei T&C. The same way, I don't think OVH logo should be committed in the repo.

I would set up a deploy_templates folder in the deployment folder in which some default customizable templates can be placed. These could be copied into ramp-frontend/ramp_frontend/templates at deployment (or somewhere else where flask can read from) and re-copied any time we change them. These could be then sent to index or sign_up through the flask code. I think we don't even have to restart the frontent since the flask could read these files dynamically.

@maikia if you set this up for the logo section of index.html, I can take care of the T&C on the sign_up page.

@maikia maikia changed the title Ovh logo [wip] Ovh logo Dec 3, 2019
@maikia
Copy link
Contributor Author

maikia commented Dec 3, 2019

@kegl @agramfort
Now the logos are taken directly from static/img/powered_by/ directory. No more hardcoded logos
If that's what you wanted I will add it to the docs. Not sure how to add a test for it though.

@agramfort
Copy link
Collaborator

nice, simple and very elegant !

+1 for MRG

feel free to MRG @kegl

@kegl
Copy link
Collaborator

kegl commented Dec 3, 2019

Nice! Maybe add the folder to .gitignore and add documentation.

@maikia maikia changed the title [wip] Ovh logo Ovh logo Dec 4, 2019
@maikia
Copy link
Contributor Author

maikia commented Dec 4, 2019

I think it's better to .gitignore the contents of the powered_by directory and not of the directory itself

@maikia maikia changed the title Ovh logo [MRG] Ovh logo Dec 4, 2019
@agramfort
Copy link
Collaborator

good to go form my end

@kegl
Copy link
Collaborator

kegl commented Dec 4, 2019

Great job @maikia, +1 for MRG

@maikia maikia merged commit 4bc503b into paris-saclay-cds:master Dec 4, 2019
@maikia
Copy link
Contributor Author

maikia commented Dec 4, 2019

Great, thanks @agramfort @kegl

return render_template('index.html')
img_ext = ('.png', '.jpg', '.jpeg', '.gif', '.svg')
images = [f for f in os.listdir(
'ramp-frontend/ramp_frontend/static/img/powered_by/')
Copy link
Collaborator

Choose a reason for hiding this comment

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

This path should be with respect to __file__, otherwise it does not exist #300

images = [f for f in os.listdir(
'ramp-frontend/ramp_frontend/static/img/powered_by/')
if f.endswith(img_ext)]
print(images)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also maybe remove the print?

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