-
Notifications
You must be signed in to change notification settings - Fork 56
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
Build static web page #559
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work! 🚀
The responsiveness is really nice and you've identified a hole in the design (the tall tiles).
I've just got some aesthetic suggestions, and I'll request a review from @javabster for the code and to correct me on anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this is looking great! One general piece of feedback I have is a lot of the padding and spacing values you used looked like they were chosen randomly, but wherever possible we should be using values consistent with IBM carbon's recommendations. For example, it's unusual to have padding of 13px, instead you should use 12 or 16.
The carbon website has a lot of good information about standards for spacing, typography, colours etc. so I recommend bookmarking it and double checking the frontend code you're writing follows their guidelines. The standards are generally pretty good and keeping in line with them is important for brand consistency and web accessibility reasons 😄
docs/style.css
Outdated
display: block; | ||
} | ||
|
||
/* ##### Read more button ##### */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice use of comments to separate CSS section 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using this style to make it even easier to read:
/* ------------------------------
* Read more button
* ------------------------------ */
<!--⚠️ If you do not respect this template, your pull request will be closed.⚠️ Your pull request title should be short detailed and understandable for all.⚠️ Also, please add a release note file using reno if the change needs to be documented in the release notes.⚠️ If your pull request fixes an open issue, please link to the issue. - [ ] I have added the tests to cover my changes. - [ ] I have updated the documentation accordingly. - [ ] I have read the CONTRIBUTING document. --> ### Summary The new web page will sort projects into different groups. This PR adds the field to the TOML files. This is just a first draft to unblock #559, the groups will probably be subject to change. Feel free to leave any comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really awesome work! The page is quick to load, and I'm impressed with how little Python code was needed.
I've left some really small suggestions but honestly I'd be happy to merge this as-is.
Co-authored-by: Frank Harkins <frankharkins@hotmail.co.uk>
Co-authored-by: Frank Harkins <frankharkins@hotmail.co.uk>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! I don't think you want to rename the current docs/
folder to contributing-docs
; that will break any links people have.
Do you need those 3 new files in docs/
to live there? Could they be in something like site/
or even ecosystem/
?
--
We probably want to make clear that ecosystem/html
has templates. Either by naming the folder html_templates
, or the individual files card.html.jinja
: https://jinja.palletsprojects.com/en/2.11.x/templates/#template-file-extension. I like the file name approach because it makes clear to IDEs/GitHub and each individual file reader what the file is.
docs/style.css
Outdated
display: block; | ||
} | ||
|
||
/* ##### Read more button ##### */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using this style to make it even easier to read:
/* ------------------------------
* Read more button
* ------------------------------ */
count_read_more = 1 | ||
max_chars_description = 500 | ||
for _, repo in projects.items(): | ||
# Card tags |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code to create a single card would be good in a helper function. It will make the overall logic easier to read.
I know you're changing the count_read_more
variable, which is global. The clearest approach would be to have your function return something that indicates the read more was used, and then whatever calls your helper function updates count_read_more
accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After our conversation, we decided to keep the code inside the build_website
method. The creation of the cards is not complex enough to read, and a helper function won't be called from any other method in the file. That auxiliary function would have to return two values, the card itself and a boolean to inform when the read-more-button has been used, therefore it can be less readable at the end.
Another point in favor of the helper function is that it will be more easy to test. Right now we can only create a test for the creation of the entire website and not for each card. Regardless this is true and we don't follow the DRY programming ideology, we think that this function doesn't need a test because there aren't many things that can fail, and in addition, the website is simple enough to just open the result (index.html
) and see if all worked.
Thank you for the review @Eric-Arellano! I've changed the name of the templates folder and the extension for each template. Now it's more clear like you said 👍. As for the docs folder, I tried having another folder but I'm not sure if we are able to change the directory for the GitHub Pages. In the configuration, the two options available are |
For classic pages it needs to be in |
@arnaucasau
|
Great! Thank you so much for your help @frankharkins and @mickahell! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job!
I'm getting a 404 :/ @arnaucasau As you don't use the |
Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>
<head> | ||
<meta charset="UTF-8" /> | ||
<meta name="viewport" content="width=device-width, initial-scale=1.0" /> | ||
<title>Qiskit Ecosystem</title> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could add the description now: #569. Frank gave a suggestion of what text to use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! 👍
Looks great at https://arnaucasau.github.io/ecosystem/! Arnau got the GitHub action working. CI was broken before this, so I'm going to bypass protections and merge. Thanks Arnau for the excellent work! |
@mickahell I set the option to publish with an action in my repo. The link should be okay now, even though it was only for this PR, and I'll probably disable it soon. Thanks again for the help! |
@arnaucasau looks very nice :D |
Summary
This PR is a continuation of the work done by @frankharkins in #548.
Things to add in upcoming pull requests:
The web page can be generated running
tox -e website
and it will be automatically deployed to GitHub Pages after every push to the main branch.View the generated page at https://arnaucasau.github.io/ecosystem/