-
Notifications
You must be signed in to change notification settings - Fork 6
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(client): add hero section landing page anonymous users (#2885) #2890
feat(client): add hero section landing page anonymous users (#2885) #2890
Conversation
You can access the deployment of this PR at https://renku-ci-ui-2890.dev.renku.ch |
1bcc601
to
ea8b360
Compare
c93f7db
to
cdf1ac2
Compare
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.
I really like this new section!
The code looks good overall! There are a few things to fix; I added comments inline. Mainly just minor classes/styles improvements, apart from a problem with one of the SVG files.
Here is another comment, still in the category "minor classes/styles improvements" but it doesn't fit inline.
The Renku logo on top looks misaligned on the left, particularly on smaller screens. The PR doesn't directly introduce this, but it's more evident now that the text in the hero section is left-justified. Adding a left margin on the logo is enough to get it aligned with the rest of the application -- here is a GIF to better explain what I mean
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.
There are a few things to adjust with this image:
- That's actually a rather heavy image for the size; looking closer, the SVG seems wrong. If you open it with a text editor, you will notice it includes an image
image id="image0_301_201" width="4096" height="2560"
while the parent SVG element issvg width="1400" height="594"
. For comparison, a correctly generated SVG looks like "Visual_Head.svg" (or the other new SVG in this PR) and should include a bunch ofg
andpath
elements instead of animage
. Could you re-export that as native SVG? - We should still upload the solid image and play with the CSS property
opacity
if we need semi-transparent images. I know there is nobackground-opacity
and that's tedious when using images as backgrounds, but there are many easy pure CSS "workarounds" (E.G: this) - (non-blocking): this image and "Visual_Head.svg" seem to overlap quite a bit. A better approach would be uploading one image and taking the desired slice. Ofc that's not always the best solution, but I feel it would work well here because the 2 single images would be bigger, and they are shown on the same page (i.e. we never load just 1). This is non-blocking, but something to keep in mind in these cases
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.
Not sure about the differentiation between "Assets" and "Graphics" here. This isn't blocking for this PR, but since you are restructuring the landing page, I think we can move all the images together, perhaps moving all the files from the Assets
folder to the Graphics
folder -- they are all SVG images
@@ -96,7 +96,7 @@ type SearchInputFormFields = { | |||
phrase: string; | |||
}; | |||
|
|||
function SearchInput() { | |||
export function SearchInput() { |
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.
This isn't used anywhere else. Should we remove it instead of exporting it, or do you plan to use it later?
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.
Yes, I keep it because there is a high probability of using it in another 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.
Ok 👍
Could you add a comment, something like "Currently not used; planned for #shapeup-issues"? We can later remove the comment once the function is used. Should we not use it, the comment will remind us why we kept this.
.btn-secondary { | ||
-#{$prefix}btn-active-bg: #{$rk-green-highlight}; | ||
} |
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.
At line 132 there is already something just for .btn-secondary
. Should we merge the code? It will be something like:
.btn-secondary {
-#{$prefix}btn-active-bg: #{$rk-green-highlight};
&:focus { ... }
}
cdf1ac2
to
0cd0bd1
Compare
0cd0bd1
to
9d42e59
Compare
@lorenzo-cavazzi Thanks for your review 🙏🏼 |
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.
Looks great! 🚀
b08456d
to
1d00d96
Compare
… screens, and remove unused graphics.
1d00d96
to
cd9d471
Compare
cb64b75
into
3357-redesign-landing-page
Tearing down the temporary RenkuLab deplyoment for this PR. |
* feat(client): add hero section landing page anonymous users (#2885) (#2890) * feat(client): add get started & Who is Behind sections landing page anonymous users (#2891) (#2896) * feat(client): Add Teaching & What Is Renku sections landing page (#2911)(#2912) (#2913) * fix(client): Add project path and dataset slug home parameters for the landing page Fix #2895 #2891 #2897 #2911 #2912
PR to add new hero section for landing page anonymous users.
Note: Get Started btn is covered in the issue #2891
Fix #2885
/deploy renku=2891-get-started-section extra-values=global.renku.cli_version=2.7.0 #persist