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

feat: align website style with www.rust-lang.org for consistency #4080

Merged
merged 5 commits into from
Dec 2, 2024

Conversation

Rustin170506
Copy link
Member

@Rustin170506 Rustin170506 commented Nov 16, 2024

👋 Long time no see!

close #3191

image

I tried to match the colors and fonts from www.rust-lang.org and make the design more consistent and modern.

  1. Use the font from www.rust-lang.org to render the title.
  2. Change the box background color to white for a cleaner and more modern look.
  3. Ensure the website is more responsive and resizable across various screen sizes.
  4. Update the link color from #428bca to #0b7261, matching the color scheme of www.rust-lang.org.

You can use a server to host the www dir to check it. For example:

npm install http-server -g

cd www

http-server    

If you have any ideas about making it more beautiful, please let me know and feel free to comment.

Copy link
Member Author

@Rustin170506 Rustin170506 left a comment

Choose a reason for hiding this comment

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

🔢 Self-check (PR reviewed by myself and ready for feedback.)

Copy link
Member

@rami3l rami3l left a comment

Choose a reason for hiding this comment

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

Thanks a bunch for making this!

Sorry for my not being a webdev expert, but TBH the deep-colored box looks weird to me. https://www.rust-lang.org uses full-width stripes instead of boxes, so I imagine something like:

image

In terms of the use of fonts and colors, I have a more radical proposal: maybe consider borrowing the style of the first and the last stripe?

Modding the https://www.rust-lang.org page for illustration purposes below (the "sponsor" section should be replaced by the command with a copy button of #ffc832):

image image

@Rustin170506

This comment was marked as outdated.

Rustin170506

This comment was marked as outdated.

@Rustin170506 Rustin170506 requested a review from rami3l November 17, 2024 09:20
@Rustin170506

This comment was marked as outdated.

@Rustin170506

This comment was marked as outdated.

@Rustin170506

This comment was marked as outdated.

Copy link
Member

@rami3l rami3l left a comment

Choose a reason for hiding this comment

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

  1. I will try to use the layout of www.rust-lang.org.
  2. I kept the box layout because we don't have enough text content; using full width would look too empty.
  3. I also tried adapting the same navigator from www.rust-lang.org, but it doesn’t work well with our content. So, I prefer not to include it.

@Rustin170506 Thanks! That looks much better indeed, but still, I think the "Run the following" line should be joining the "Get Started" one, that is, both left-aligned, either outside or inside the box (I guess outside would look more natural).

I also formatted the index HTML in a separate commit and added the missing closing body tag. Let me know if you'd prefer not to format it until we establish a standard formatter. I used the default HTML formatter in VS Code.

I have no problem with that, as long as the formatting is made in a single commit for maximal clarity.

cc @djc @rbtcollins

@Rustin170506
Copy link
Member Author

Rustin170506 commented Nov 17, 2024

think the "Run the following" line should be joining the "Get Started" one, that is, both left-aligned, either outside or inside the box (I guess outside would look more natural).

The reason I didn't do that is that when you click display all..., the content changes, and more items are added to the box. So it doesn't make sense to link only the first one to Get Started.

Thanks for your review! 💚 💙 💜 💛 ❤️

@Rustin170506
Copy link
Member Author

Rustin170506 commented Nov 17, 2024

@rami3l What do you think if we remove that get started? It seems a little bit weird.

image

It seems removing it would make it simpler and clearer.

@rami3l
Copy link
Member

rami3l commented Nov 17, 2024

@Rustin170506 Hmmm honestly speaking I'm not sure... Let me take some time to think about it.

@Rustin170506
Copy link
Member Author

Rustin170506 commented Nov 17, 2024

@Rustin170506 Hmmm honestly speaking I'm not sure... Let me take some time to think about it.

No rush—please take your time. I believe we should consider removing it because the main content is concentrated in the middle of the page. The short Get Started section feels out of place within the overall layout. Its brevity makes the page look unbalanced and less cohesive.

The main reason I didn’t follow the two-column layout from www.rust-lang.org is that we don’t have enough content to fully utilize two columns. Keeping the content centered makes the layout cleaner and more visually balanced.

@djc
Copy link
Contributor

djc commented Nov 19, 2024

IMO this PR is a little tricky because it contains a whole bunch of changes at the same time. Although I understand that design maybe a little different in that it might be harder to get to a good (consistent) design by smaller pieces, I think in this context it would be helpful to split the changes here into a series of smaller ones that can be judged separately.

For example, I do really like the idea of using the font and accent color from the Rust website here, but we can make smaller/targeted changes for that.

There also seem to be a whole bunch of non-functional formatting changes here which IMO it would be good to isolate separately and decide how we want to handle CSS formatting (ideally with automated tools) -- personally for example the change from > to > in selectors does not look like an improvement to me.

For other structural changes to the HTML, I'd also like to see those isolated so we can discuss their merits separately.

@Rustin170506
Copy link
Member Author

Rustin170506 commented Nov 19, 2024

There also seem to be a whole bunch of non-functional formatting changes here which IMO it would be good to isolate separately and decide how we want to handle CSS formatting (ideally with automated tools) -- personally for example the change from > to > in selectors does not look like an improvement to me.

I can separate these changes to a separate commit. Or we can chose to do not include any format changes at all. I can do it in the following PRs.

@djc
Copy link
Contributor

djc commented Nov 19, 2024

Separate commits are helpful but, to the extent it doesn't cause outsized rebase hazards I think I would prefer separate PRs here so we can merge changes independently of each other.

@Rustin170506
Copy link
Member Author

There also seem to be a whole bunch of non-functional formatting changes here which IMO it would be good to isolate separately and decide how we want to handle CSS formatting (ideally with automated tools) -- personally for example the change from > to > in selectors does not look like an improvement to me.

I have created an issue for it. #4091 If you have more thought on it, we can discuss it in the issue.

I will cleanup this PR later.

@Rustin170506 Rustin170506 force-pushed the rustin-patch-www branch 4 times, most recently from 910b826 to e6d3015 Compare November 19, 2024 12:16
Copy link
Member Author

@Rustin170506 Rustin170506 left a comment

Choose a reason for hiding this comment

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

🔢 Self-check (PR reviewed by myself and ready for feedback.)

@Rustin170506
Copy link
Member Author

Rustin170506 commented Nov 19, 2024

@rami3l @djc I believe this PR is ready for another review. If you'd prefer to split it further, I think the only thing we could extract is the color styling for the <a> tag. The rest pertains to the overall look of the website, and I don't think it makes sense to separate those into different PRs.

@Rustin170506 Rustin170506 requested a review from rami3l November 19, 2024 12:26
@Rustin170506 Rustin170506 force-pushed the rustin-patch-www branch 2 times, most recently from d9af961 to da194fa Compare November 19, 2024 13:30
@Rustin170506 Rustin170506 requested a review from djc November 19, 2024 13:33
@Rustin170506
Copy link
Member Author

@djc I’ve split each change into a single commit. Apologies for any confusion caused by unclear changes.

@Rustin170506 Rustin170506 force-pushed the rustin-patch-www branch 2 times, most recently from 838dff7 to 97b93c4 Compare November 19, 2024 13:45
Copy link
Member Author

@Rustin170506 Rustin170506 left a comment

Choose a reason for hiding this comment

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

@djc Here are the reasons behind the decisions I made. I hope this helps you with the review.

www/index.html Show resolved Hide resolved
www/index.html Show resolved Hide resolved
www/rustup.css Show resolved Hide resolved
www/rustup.css Show resolved Hide resolved
www/rustup.css Show resolved Hide resolved
www/rustup.css Show resolved Hide resolved
www/rustup.css Show resolved Hide resolved
@djc
Copy link
Contributor

djc commented Nov 25, 2024

@Rustin170506 I was going to review the current state of the PR this morning, are you sure you want to retract it? Seems like a waste to me!

@Rustin170506
Copy link
Member Author

I was going to review the current state of the PR this morning, are you sure you want to retract it? Seems like a waste to me!

Thank you for your time! I'm a bit hesitant about this change. As the issue author mentioned, it's hard to define what makes a "good-looking website." So I'm a bit worried that this change (from a UI design perspective) might be seen by others not as an improvement but as a step backward.

@Rustin170506
Copy link
Member Author

Maybe we should wait until a designer (or someone with more web design experience) is willing to provide a completely new design for the website before making adjustments to the implementation.

@djc
Copy link
Contributor

djc commented Nov 25, 2024

I think any changes that move the rustup website closer to the main Rust website in fairly obvious ways would be a clear improvement, so I still think there's value in incremental changes like this.

@Rustin170506
Copy link
Member Author

I think any changes that move the rustup website closer to the main Rust website in fairly obvious ways would be a clear improvement, so I still think there's value in incremental changes like this.

Cool! I've reopened it. If there's anything I can do to help review the changes, please let me know!

@Rustin170506 Rustin170506 reopened this Nov 25, 2024
www/rustup.css Show resolved Hide resolved
www/rustup.css Show resolved Hide resolved
@rami3l
Copy link
Member

rami3l commented Nov 28, 2024

@Rustin170506 Don't worry about the CI failure; I'm working on this (#4098 (comment)).

@rami3l rami3l added this to the 1.28.0 milestone Nov 29, 2024
Copy link
Member

@rami3l rami3l left a comment

Choose a reason for hiding this comment

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

This looks pretty nice! Thanks a lot for your help :)

@rami3l
Copy link
Member

rami3l commented Dec 2, 2024

@djc I guess it's not against the rules to merge this immediately so that it can get to the beta as well (after #4098, of course)?

@djc djc added this pull request to the merge queue Dec 2, 2024
Merged via the queue into rust-lang:master with commit 14470b2 Dec 2, 2024
27 checks passed
@Rustin170506
Copy link
Member Author

Thanks for your review! 💚 💙 💜 💛 ❤️

@Rustin170506 Rustin170506 deleted the rustin-patch-www branch December 2, 2024 10:52
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.

Rustup.rs needs some rust theming for credibility
3 participants