Skip to content
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

style: reduce company logo size on casestudy/[id] page #2728

Merged
merged 14 commits into from
Mar 11, 2024
Merged

style: reduce company logo size on casestudy/[id] page #2728

merged 14 commits into from
Mar 11, 2024

Conversation

RamGoel
Copy link
Contributor

@RamGoel RamGoel commented Mar 3, 2024

Description

  • This PR refactor the CSS of /casestudies/[id] page and replaces any fixed width with relative width in tailwind classes.
  • This gives the article proper spacing from both ends.

Related issue(s)
Resolves #2727

Preview

image

Copy link

netlify bot commented Mar 3, 2024

Deploy Preview for asyncapi-website ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 4011d6d
🔍 Latest deploy log https://app.netlify.com/sites/asyncapi-website/deploys/65ee76e98eb6840008f4af8b
😎 Deploy Preview https://deploy-preview-2728--asyncapi-website.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.

@asyncapi-bot
Copy link
Contributor

asyncapi-bot commented Mar 3, 2024

⚡️ Lighthouse report for the changes in this PR:

Category Score
🔴 Performance 37
🟢 Accessibility 98
🟢 Best practices 92
🟢 SEO 100
🔴 PWA 33

Lighthouse ran on https://deploy-preview-2728--asyncapi-website.netlify.app/

@anshgoyalevil
Copy link
Member

This new indentation doesn't look consistent with the one we had earlier:

image

@anshgoyalevil
Copy link
Member

anshgoyalevil commented Mar 4, 2024

Also, would request you to wait for a triager to approve the issue before starting to work on it :)

@RamGoel
Copy link
Contributor Author

RamGoel commented Mar 4, 2024

Also, would request you to wait for a triager to approve the issue before starting to work on it :)

Approve? you mean 'to assign'?

@sambhavgupta0705
Copy link
Member

@RamGoel It doesn't look good in mobile view and IMO we don't need a change like this
@akshatnema @anshgoyalevil what are your thoughts on this

@sambhavgupta0705
Copy link
Member

@RamGoel we can decrease the size of the company image which will look better
can you please do that

@RamGoel
Copy link
Contributor Author

RamGoel commented Mar 5, 2024

@RamGoel we can decrease the size of the company image which will look better can you please do that

yes sure.

@RamGoel
Copy link
Contributor Author

RamGoel commented Mar 5, 2024

This new indentation doesn't look consistent with the one we had earlier:

image

@sambhavgupta0705 I decreased the image size from 300px to 250px and added the indentation fix as well.

@RamGoel
Copy link
Contributor Author

RamGoel commented Mar 7, 2024

Hey guys! Could you please review this.

@RamGoel
Copy link
Contributor Author

RamGoel commented Mar 7, 2024

This new indentation doesn't look consistent with the one we had earlier:

image

@sambhavgupta0705 as @anshgoyalevil suggested that the indentation earlier was not looking good. so I added this isAgain thing to make the indentation consistent.

Here is updated indentation, this is same as we have earlier, that is why you aren't able to notice

image

const typeStyle =
level === 0 ? "heading-lg" : level === 1 ? "heading-md" : "heading-sm";

return content.map((item) => {
return (
<div
className="mt-10"
className={`mt-10 mx-auto ${!isAgain?'w-11/12':''}`}
Copy link
Member

@sambhavgupta0705 sambhavgupta0705 Mar 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RamGoel I am not able to understand the use of isAgain
So please remove it

@sambhavgupta0705
Copy link
Member

@RamGoel just decrease the size of the image for this PR

@@ -17,7 +17,7 @@ const renderContent = (content, allComponents, level) => {
return content.map((item) => {
return (
<div
className="mt-10"
className={`mt-10 mx-auto w-11/12`}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rewrite this in conventional tailwind syntax as it was written previously

Copy link
Contributor Author

@RamGoel RamGoel Mar 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you want to remove mx-auto w-11/12 as well? or just remove {} as there is no condition checking anymore?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just the braces

@RamGoel RamGoel closed this Mar 7, 2024
@RamGoel RamGoel reopened this Mar 7, 2024
Copy link
Member

@sambhavgupta0705 sambhavgupta0705 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM cc: @anshgoyalevil

@sambhavgupta0705
Copy link
Member

@RamGoel you have made the changes in indentation again
Please revert them

Copy link
Member

@sambhavgupta0705 sambhavgupta0705 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RamGoel please revert the indentation change

@RamGoel
Copy link
Contributor Author

RamGoel commented Mar 7, 2024

@RamGoel please revert the indentation script

The whole idea of this PR is fixing the spacing, so I used w-11/12 mx-auto to maintain proper spacing on both sides, but as the renderContent function is calling recursively, we need the class w-11/12 mx-auto only for the first call.

That is why I had added the isAgain thing. It was to remove the indentation itself.

@sambhavgupta0705
Copy link
Member

Screenshot_20240307-195236.png

This is the current indentation we don't want any change in this
We just need change in the image size

@RamGoel
Copy link
Contributor Author

RamGoel commented Mar 7, 2024

image

For me it looks like this, on Win11, Chrome Zoom 100% . That was the reason I raised the PR. on 90% Zoom it looks good. but on 100% it sticks too much on left

@sambhavgupta0705
Copy link
Member

@RamGoel in this PR the image size has been decreased correctly but the indentation of the points has been changes which we don't want
image

Please revert this change

@anshgoyalevil
Copy link
Member

/rtm

@asyncapi-bot asyncapi-bot merged commit 37201d9 into asyncapi:master Mar 11, 2024
22 checks passed
@anshgoyalevil anshgoyalevil changed the title style: casestudy page horizontal spacing style: reduce company logo size on casestudy/[id] page Mar 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Large company logo size on casestudy/[id] page
4 participants