search

knative / docs

User documentation for Knative components.
https://knative.dev/docs/
Other
4.52k stars 1.23k forks source link

added Linkedln page link #5976

Closed shivamgupta2020 closed 3 months ago

shivamgupta2020 commented 4 months ago

Fixes #5972

Proposed Changes

netlify[bot] commented 4 months ago

Deploy Preview for knative ready!

Built without sensitive environment variables

Name Link
Latest commit 7f2b53dbfde16f56e74908e8d3ee3564856f66da
Latest deploy log https://app.netlify.com/sites/knative/deploys/6662321e38997c0007fb84d2
Deploy Preview https://deploy-preview-5976--knative.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

DhairyaMajmudar commented 4 months ago

I think it will look much better if we split the logos section into 4 columns. One column for each logo.

Also while hovering it shows the logo into greenish gradient which can be made better if we change the color into the actual logo color (egs: black for Twitter logo) or we can also change the logo color into color matching Knative logos while hovering.

cc: @aliok @shivamgupta2020

shivamgupta2020 commented 4 months ago

Can you please revert the formatting changes that are not relevant to the task?

Also, can you add some spacing between X and LinkedIn logos?

@aliok I have updated the linting of code and add some space between the logo. Updated Look - Screenshot from 2024-05-29 04-47-02

shivamgupta2020 commented 4 months ago

I think it will look much better if we split the logos section into 4 columns. One column for each logo.

Also while hovering it shows the logo into greenish gradient which can be made better if we change the color into the actual logo color (egs: black for Twitter logo) or we can also change the logo color into color matching Knative logos while hovering.

cc: @aliok @shivamgupta2020

Hi @DhairyaMajmudar , I think splitting the section into four parts isn't necessary since LinkedIn and Twitter perform the same task. Also, the color change on hover hasn't been decided by the UX team yet, so we should hold off on that for now. However, I like your suggestion to use the actual logo color on hover.

aliok commented 3 months ago

@shivamgupta2020

If you remove the irrelevant formatting changes, we can merge this PR. Thanks!

shivamgupta2020 commented 3 months ago

@shivamgupta2020

If you remove the irrelevant formatting changes, we can merge this PR. Thanks!

@aliok, I've reverted the formatting changes. Please review it on your machine.

aliok commented 3 months ago

@shivamgupta2020

When I check https://github.com/knative/docs/pull/5976/files I see they are still there.

shivamgupta2020 commented 3 months ago

@aliok, I've reverted the formatting changes. Please review it.

nainaz commented 3 months ago

Looks good to me. I like the space between X and in. Maybe it is just my eyes, in looks smaller like this :) Thank you again @shivamgupta2020!

aliok commented 3 months ago

Thanks @shivamgupta2020

It looks great.

About the previous comments about colors, etc. Let's address them separately as we need to change those everywhere.

knative-prow[bot] commented 3 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aliok, shivamgupta2020

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/knative/docs/blob/main/OWNERS)~~ [aliok] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment