-
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
Updates to Anonymous Landing Page #2741
Conversation
From the meeting today:
|
16e59f2
to
e493e22
Compare
You can access the deployment of this PR at https://renku-ci-ui-2741.dev.renku.ch |
012b8a4
to
821fd7f
Compare
1775220
to
454bf7f
Compare
Testing: check that the new design matches Figma |
</Col> | ||
<Col md={3}> | ||
<BottomNavSection sectionTitle="Follow us"> | ||
<BottomNavExternalLink title="Twitter" url={Links.TWITTER} /> |
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.
🤓
<BottomNavExternalLink title="Twitter" url={Links.TWITTER} /> | |
<BottomNavExternalLink title="𝕏 (formerly Twitter)" url={Links.TWITTER} /> |
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 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 looks good! A couple of tiny changes before merging:
-
The buttons margin/spacing could be improved by removing the right margin and adding X margins, and adding either the bottom or Y margins. Currently, they have no bottom margin and the first 2 buttons have a right margin, making their borders collapse together on smaller screens and moving them into a non horizonatally centered position.
-
Bottom margins on the top container are inconsistent, depending on the browser size
-
See inline typos
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.
About styling: I would suggest avoiding global CSS definitions and using Bootstrap's utility classes as much as possible. For cases when custom styling is required, (S)CSS modules seem appropriate as they will not affect components outside of their scope.
@@ -135,6 +135,18 @@ $borderRadius: 1000px; | |||
} | |||
} | |||
|
|||
.btn-outline-rk-white { |
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 class is generated by Bootstrap, I would prefer if we do not change its behavior globally. Would it be possible to override this in a (S)CSS module?
Also, overriding the color can be done with the bootstrap CSS variables (e.g. check the overrides from btn-danger
or btn-outline-danger
).
@@ -100,6 +114,17 @@ nav.rk-anon-home .nav-link { | |||
} | |||
} | |||
|
|||
#rk-anon-home-bottom-nav { | |||
@extend .section-content, .bg-primary; | |||
color: $rk-white; |
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.
Use text-rk-white
as a class name.
b36a24d
to
7868780
Compare
7868780
to
1d2dea1
Compare
Thanks for the reviews, it's important that this page be polished. I think I have addressed all requests. |
@@ -176,6 +164,16 @@ $borderRadius: 1000px; | |||
--bs-btn-active-color: white; | |||
} | |||
|
|||
.btn-rk-over-green { | |||
--bs-btn-active-bg: #{$rk-green}; |
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.
As a note, I think var(--bs-rk-green)
would work too.
</Col> | ||
<Col md={3}> | ||
<BottomNavSection sectionTitle="Follow us"> | ||
<BottomNavExternalLink title="Twitter" url={Links.TWITTER} /> |
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.
Co-authored-by: Flora Thiebaut <johann.thiebaut@sdsc.ethz.ch>
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.
Lgtm.
Keep using "Twitter" works fine for me since the new name is hard to relate to the product without the icon or specifying that it's actually what used to be Twitter
Tearing down the temporary RenkuLab deplyoment for this PR. |
An update to the landing page.
Fix #2681
/deploy renku=renku-ui/3.11-acceptance-tests #persist