-
-
Notifications
You must be signed in to change notification settings - Fork 525
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: Rebrand the homepage(colors and hero-brand) #1009
Conversation
Can you rebase the website with |
7138fc2
to
6d1180b
Compare
For the time being, yes please.
We can do that later in a different PR. What do I need to upload?
We have a script that applies the randomness. I suppose you want to do something similar: biome/website/src/frontend-scripts/index.ts Lines 25 to 31 in d6a0923
|
Sure, you can upload screenshot in resources. |
We can do that later :) What's missing? I am very exited to start merging things! |
ddd41a6
to
d2f2d1c
Compare
Refactor code according to required changes and also apply some media queries, check now. |
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.
Thank you for starting things! Here's some advice on how to do better PRs and help us to make god reviews, and some feedback for this PR:
- in general, in open source, it's better to do small and localized PRs
- my suggestion is to break down this PR in smaller ones, for example:
- new logo and colours
- contributor component
- the performance component
- we are adding all new assets and they are not used anywhere and I can't understand why; we should add only those images that we use. No need to create a new folder, we can replace the ones the currently have (or delete them)
- you have a typo in folder,
srcipts
instead ofscripts
. Also, we already have a folder for that, calledfrontend-scripts
- please remove the images we don't use;
Maybe, as a starting point, we can make this PR as a colour change and new logo. What do you think?
const updateProgress = () => { | ||
const elapsed = Date.now() - startTime; | ||
const newProgress = elapsed / 1000; | ||
console.log(newProgress, "p"); |
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.
to remove
margin-top: 20px; | ||
width: 50px; | ||
margin-left: 10px; | ||
font-size: 15px; |
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 general, I am not a fan fan of px
, unless they are really needed. For example, font sizes can use rem
or em
. Can we use rem
or em
instead?
.bar { | ||
height: 100%; | ||
transition: width 0.1s ease-in; | ||
background-color: #60a5fa; |
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.
For colours and sizes, we should use starlight colours, or create our own CSS variables and use them here.
website/src/styles/_overrides.scss
Outdated
--sl-community-link: #ffffff; | ||
} | ||
|
||
.hero.astro-jbfsktt5 { |
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.
These styles that have strange letters are auto-generated, which means that they might change in the future. We can't override the styles like this.
d926fbf
to
0542720
Compare
@ematipico, thanks for the guidance, now this PR only targets the |
bc62275
to
e7874fb
Compare
e337ca3
to
0161296
Compare
Can you check the new version? |
Still nothing, but we can figure it out later :) I think for now we can merge it and continue with other works. Thank you! |
Summary
cc @ematipico
Test Plan