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

fix(kno-7538): update images to use Image component #723

Open
wants to merge 38 commits into
base: main
Choose a base branch
from

Conversation

rachael-t
Copy link
Contributor

@rachael-t rachael-t commented Dec 23, 2024

Description

Many of the images used throughout the docs did not follow our style guide and use the <Image> component (with the classes className="rounded-md mx-auto border border-gray-200")

This PR updates those image references to use the component. In the cases where an image was either very outdated, or either had a border in the image itself, I created and added a new image.

Note that I did not update .gif references to use the <Image> component since the component is designed for static image optimization.

From this work, a few other tasks have been flagged in the Docs project in Linear:

  1. Update images with template editor v3 release
  2. Add a checklist to pull_request_template.md
  3. Add CI/build rules to enforce image formatting

Todos

  • Update images that were flow charts or tables - will not use the border border-gray-200 classes
    • Confirm with team that's how we want to handle these types of images, and if so, update style guide
  • Update images that are of a full browser window (and therefore have formatting applied) - will not add any classes
    • Confirm with team that's how we want to handle these types of images, and if so, update style guide
  • Triple check that any deleted images are not being used anywhere in the docs

Tasks

KNO-7538

Copy link

vercel bot commented Dec 23, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 26, 2024 6:46pm

Copy link

linear bot commented Dec 23, 2024

@@ -99,15 +99,14 @@ If you are passing a JSON payload to the webhook, you can use the `json` filter

Now we can build our notification workflow that sends out a webhook when the customer has a webhook configured for the object.

Start by creating a new workflow in Knock:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regarding the changes below, see KNO-7573

@rachael-t rachael-t changed the title [DRAFT] fix(kno-7538): update images to use Image component fix(kno-7538): update images to use Image component Dec 26, 2024
@rachael-t rachael-t marked this pull request as ready for review December 26, 2024 19:08
@rachael-t
Copy link
Contributor Author

I'd like the team to weigh in on the following decisions I made for certain image types. Based on what we decide, I will update the style guide.

  1. Images that are flow charts/tables should not use the border border-gray-200 classes
  2. images that are of a full browser window (and therefore have formatting applied) will not have any classes applied

@rachael-t
Copy link
Contributor Author

I realized that the Fetch function page has double borders (and the UI in the images is outdated). However, I will need to make that update once I have access to a larger monitor for better screenshots (Loom of me explaining).

Copy link
Contributor

@cellomatt cellomatt left a comment

Choose a reason for hiding this comment

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

Marking as approved so you can move forward, I confirmed that all of the images (including renamed ones) are displaying properly.

With that being said -- I think we should review the usage of width/height across all of these images, and also come up with some guidelines for their use to add to the style guide. It's unclear to me how exactly they are used by Next, as their documentation says that they are not CSS styles. However, in my own testing they do seem to impact the way that images are rendered.

I think these values should probably match the dimensions of the original image, or at least fractionally (like 400 for an image with 800px width, for example). I also don't love the way that a lot of these images look when they render 100% width on the content column, and think it could be worth considering when/if to constrain the width or height of certain images

Comment on lines +45 to +46
width={800}
height={450}
Copy link
Contributor

Choose a reason for hiding this comment

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

Question about these (and similar usages throughout): are we getting these values from the actual image files? In this particular case, it seems like they don't align with the actual shape of the image?

It seems like these values don't determine the actual dimensions of the image, but it's probably good for them to be approximately accurate?

content/in-app-ui/overview.mdx Show resolved Hide resolved
src="/images/integrations/extensions/connect-segment-modal.png"
width={800}
height={450}
className="rounded-md mx-auto border border-gray-200"
Copy link
Contributor

Choose a reason for hiding this comment

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

One thing I wonder about is whether we might want to add a constrained width to some of these images (esp. relevant here because the original had that).

Supposedly the height/width props on the Image component are not used to style the image, but I tried setting width={400} on this one locally and it did constrain it. Sometimes we have images like this one that look kind of oversized when rendered to the full column width.

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.

2 participants