-
Notifications
You must be signed in to change notification settings - Fork 61
New Get AlmaLinux page design #848
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
base: master
Are you sure you want to change the base?
Conversation
bennyvasquez
left a comment
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.
In addition to the couple things that I pointed out that you'll want to adjust for throughout, this PR doesn't seem to use any of the boostrap options for layout. Please adjust to use boostrap classes for layouts. This is ultimately cleaner and leads to reduced work for dynamic/mobile displays, as well as making it easier to understand the impact that they layout choices have. For example:
https://getbootstrap.com/docs/4.0/utilities/flex/
https://getbootstrap.com/docs/4.0/utilities/spacing/
You can see examples of that on this page:
https://github.com/AlmaLinux/almalinux.org/blob/master/layouts/members/single.html
|
@bennyvasquez hi! I think I've updated the most to replace inline code with bootstrap, if you have a moment to take a look if that's the right direction. Responsive design still requires polishing so meanwhile I'm going to finish with it |
Pull request summary
|
|
@sboldyreva this looks much better to me! Because of my delay, though, there are some conflicts that need to be resolved. If you can get those corrected, then I'm happy to merge this in! |
|
Thank you so much @sboldyreva! I'm just about ready to merge this, but I noticed we lost both the WSL and the LXC icons from the top, which has triggered two more questions for me:
|
|
@bennyvasquez each set of OS - version - architecture has now its own respective set of icons followed by the links. The very top set relates to AlmaLinux 10.0 x86_64. At the moment of creating this pull request, there were no available WSL and Incus and LXC images for them. That led to removing their icons from the set. When updating versions&URL to the newest release, along with the team, I will check available images once again and place available on the download page. Meaning that if WSL and Incus and LXC are now available for any AlmaLinux version (10.0 or 10.1) and arch - the icons and link will be added to the required section :) |
|
@sboldyreva I just realized that this PR was made before the implementation of prettier, and that is likely why it's failing now. I fixed the problem in the get-almalinux file, but now it's throwing an error on the elevate page. Can you merge master into your PR and then we can see if it continues to be a problem? |
|
@sboldyreva @bennyvasquez I think the problem is a superfluous
This happens when prettier is unable to parse the structure of the file.
|
|
@bennyvasquez not sure if I have fixed that, but I made some clean up |
There might be some typos/inaccuracies to fix