-
Notifications
You must be signed in to change notification settings - Fork 5
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
Brenosalv/feature/redesign-product-page #370
Conversation
…to Brenosalv/feature/redesign-product-page
<StaticImage | ||
placeholder="none" | ||
alt="Oracle logo" | ||
src="../../../../images/logos/oracle.png" | ||
layout="constrained" | ||
/> |
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 is okay for now and needs no changes... we should think of a way to store data for things like this so that all the images, alt, and titles are always in sync. As this is a lot of code that could easily get messed up with a copy/paste.
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 but since the StaticImage
component only accept constant values from the same file for the props like src
and alt
, we can't import the data from elsewhere if it's what you mean.
src/images/logos/amazon-s3.png
Outdated
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.
What is driving saving off these logos? These are already available from the connectors table... however I guess getting those URLs could be tough right now. I think we're okay right now but should look into using the standard ones... then we get the caching benefit.
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.
Because the logos for the connectors on the integrations page differ from those in the product page section two diagram. Later we could either replace them with the ones from the integrations page or update the logos on the integrations page to ensure consistency.
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.
That's good to know... yeah those should be the same for sure. I'll open an issue for us to look into it eventually.
src/images/product-page/section-two/desktop/ai-connectors-group.png
Outdated
Show resolved
Hide resolved
…the slides height
@@ -60,6 +60,8 @@ const SectionOne = () => { | |||
} | |||
`); | |||
|
|||
const metricIconColor = '#FFFFFF'; |
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.
Sorry to be a pain... tiny change but can you declare this outside of the function.
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.
Sure, makes sense.
…to Brenosalv/feature/redesign-product-page
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.
new changes lgtm
#296
Changes