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

Fixes #1791: Handling with inline img #1910

Merged
merged 1 commit into from
Mar 23, 2021

Conversation

rjayroso
Copy link
Contributor

@rjayroso rjayroso commented Mar 12, 2021

Issue This PR Addresses

Fixes #1791
Fixes #1412

Type of Change

  • Bugfix: Change which fixes an issue
  • New Feature: Change which adds functionality
  • Documentation Update: Change which improves documentation
  • UI: Change which improves UI

Description

I piggy-backed off of @huynguyez 's solution that he posted in issue #1791 (see here) and just added an extra line vertical-align:50%; to center the inline images.

Before
Capture4
After
Capture3

Checklist

  • Quality: This PR builds and passes our npm test and works locally
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not (if applicable)
  • Documentation: This PR includes updated/added documentation to user exposed functionality or configuration variables are added/changed or an explanation of why it does not(if applicable)

@izhuravlev
Copy link
Contributor

izhuravlev commented Mar 12, 2021

I was not sure how to test this, so I opened the deployment and saw this:
image

So I assume some of the regular blogpost images are getting shrunk as well, which is not great.

@huyxgit
Copy link

huyxgit commented Mar 12, 2021

I was not sure how to test this, so I opened the deployment and saw this:
image

So I assume some of the regular blogpost images are getting shrunk as well, which is not great.

@izhuravlev Yes I mentioned this issue on Tuesday about this

Copy link

@huyxgit huyxgit left a comment

Choose a reason for hiding this comment

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

Like I talked about this in the last call.

Using telescope-post-content div will also affect those blogs from blogspot.

@izhuravlev
Copy link
Contributor

Like I talked about this in the last call.

Using telescope-post-content div will also affect those blogs from blogspot.

Oh that's what you meant, I just was not sure what blogspot is

@rjayroso
Copy link
Contributor Author

I was not sure how to test this, so I opened the deployment and saw this:
image

So I assume some of the regular blogpost images are getting shrunk as well, which is not great.

Hmm, do you think we should just keep what we have now and scrap this along with closing the addressed issue?

@izhuravlev
Copy link
Contributor

@rjayroso I think we should leave this for now, until any other idea pops up

@yuanLeeMidori
Copy link
Contributor

@rjayroso I think we should leave this for now, until any other idea pops up

Agree. I believe currently there is no one method to take care of all the cases.

@rjayroso
Copy link
Contributor Author

I removed the styling for the images to be inline as it would, unfortunately, ruin some posts by making the images too small. I'm thinking of keeping it to what we had before but implement the styling for the GitHub links so it matches the font size as the rest of the post (1.8rem). This would at least make the links look less out of place, but it doesn't really deal with the images being too big (see below):

Before (i.e. No div > h1 styling)

image

After (set to 1.8rem)

image

Copy link
Contributor

@birtony birtony left a comment

Choose a reason for hiding this comment

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

I think this image on the bottom of @yuanLeeMidori's post does not display properly:
image
image
I think we should address it in this issue @rjayroso

@rjayroso
Copy link
Contributor Author

rjayroso commented Mar 17, 2021

@birtony I checked it out and I agree the photo was not being displayed properly. I rebased and added a max-height to make tall pictures more friendly to view, see below (ignore red border):

a. Before
beforepic
a. After
afterpic
b. Before
beforepic2
b. After
afterpic2

I think the photo on the bottom of Yuan's post is a bit trippy because of how it was screenshotted, but either way let me know if you like this change otherwise I can revert it.

UPDATE: I think I finally got everything working properly:

image

Copy link
Contributor

@birtony birtony left a comment

Choose a reason for hiding this comment

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

Please, file another issue for cases like below before merging this PR @rjayroso
image

Comment on lines +26 to +32
.telescope-post-content div > a > img {
width: 32px;
display: inline;
vertical-align: middle;
}

.telescope-post-content h1 > a > img {
Copy link
Contributor

Choose a reason for hiding this comment

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

You could group these like:

.telescope-post-content div, h1 > a > img {
...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried this just now and it seems to ignore the CSS completely (as if it ain't there), I don't know why - thoughts?

yuanLeeMidori
yuanLeeMidori previously approved these changes Mar 18, 2021
Copy link

@huyxgit huyxgit left a comment

Choose a reason for hiding this comment

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

Nice for other blogs, but Blogspot image issue still there (32px).

@rjayroso
Copy link
Contributor Author

@huynguyez I can file an issue to fix the BlogSpot bug and merge this

I created another issue here of a potential fix for this

@humphd humphd requested review from huyxgit and birtony March 19, 2021 17:47
* Added styling for div img [Removed]
* Added styling for div > h1 and div > div [Removed]
* Removed styling for div > div and div img, kept styling for div > h1
* Added max-height for tall pictures
* Fixed inline images without affecting posts (BlogSpot still affected)
* Fixed prettier complaint
huyxgit
huyxgit previously approved these changes Mar 19, 2021
Copy link

@huyxgit huyxgit left a comment

Choose a reason for hiding this comment

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

Since you gonna break it into another issue for Blogspot, this looks good to me!

Copy link
Contributor

@chrispinkney chrispinkney left a comment

Choose a reason for hiding this comment

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

LGTM! Also very curious to see what happens with your blogspot idea 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: css styling Anything related to CSS styling area: front-end area: nextjs Nextjs related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handling with inline img Deal with inline images being streched (aka reduce the size of Tony)
7 participants