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.

Sign up for GitHub

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

Jump to bottom

Fixes #1370: Set picture in post as their natural size #1373

Closed
wants to merge 3 commits into from
Closed

Fixes #1370: Set picture in post as their natural size #1373

wants to merge 3 commits into from

Conversation

phast184
Copy link

@phast184 phast184 commented Nov 16, 2020 •
edited
Loading

Issue This PR Addresses

Issue #1370

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

Changes: Pictures that were not set as natural size so that they were blurry when getting displayed.
Before changes:

image

After changes:
image

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)

@phast184 phast184 changed the title fix blurry pictures in post https://github.com/Seneca-CDOT/telescope/issues/1370 fix blurry pictures in post #1370 https://github.com/Seneca-CDOT/telescope/issues/1370 Nov 16, 2020
@phast184 phast184 changed the title fix blurry pictures in post #1370 https://github.com/Seneca-CDOT/telescope/issues/1370 fix blurry pictures in post [#1370] (https://github.com/Seneca-CDOT/telescope/issues/1370) Nov 16, 2020
@phast184 phast184 changed the title fix blurry pictures in post [#1370] (https://github.com/Seneca-CDOT/telescope/issues/1370) fix blurry pictures in post Nov 16, 2020
humphd
humphd requested changes Nov 16, 2020
View reviewed changes
Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

This PR needs a number of things:

  1. Fix the first comment to add missing information. Please include the issue number this fixes in the format "Fixes #..." so that GitHub will link the issue. Please include a proper description of what you've done, as well as screenshots of the change running.
  2. Your fix applies way too narrowly. This won't deal with any images on other domains. We need a general solution here.

Also, I left a comment in the issue asking you to improve how you communicate when taking issues. Please approach this work more professionally.

@phast184 phast184 changed the title fix blurry pictures in post Fixes #1370 Nov 16, 2020
@phast184 phast184 changed the title Fixes #1370 Fixes #1370: Set picture in post as their natural size Nov 16, 2020
@phast184
Copy link
Author

phast184 commented Nov 17, 2020 •
edited
Loading

@humphd Thank you for letting me know what I'm missing. By the way, can you please have a look at the first comment to see if I have met the requirements? And I also have made some modifications to make the solution more general as you said.

humphd
humphd reviewed Nov 17, 2020
View reviewed changes
src/frontend/src/components/Post/telescope-post-content.css
@@ -19,7 +19,8 @@
max-width: 100%;
padding-top: 1.5rem;
padding-bottom: 1rem;
width: 100%;
width: auto;
display: block;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about this change to block. This is going to mess-up cases where we have inline images in posts. @cindyledev or @agarcia-caicedo might know more. Am I wrong on this?

@humphd
Copy link
Contributor

humphd commented Nov 17, 2020

Updates to first comment look good, thank you.

@humphd
Copy link
Contributor

humphd commented Nov 18, 2020

@phast184 what's happening with this? Are you hoping to include it for 0.3? If so, we need to get it in today, and there's a long line of PRs that need reviews and merges. Please co-ordinate with us on Slack and here.

phast184
phast184 commented Nov 18, 2020
View reviewed changes
src/frontend/src/components/Post/telescope-post-content.css
@@ -47,6 +47,10 @@
height: 1px;
}

.telescope-post-content img[src*='1.bp.blogspot.com/'] {
display: block;
Copy link
Author

Choose a reason for hiding this comment

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

@humphd @humphd Thank you for pointing this out. I noticed that those pictures scaled beyond their natural size belong to Blogger.com posts. Therefore, to avoid messing up inline image cases. I have push another commit in which I specifically declare that any picture from Blogger.com would be display: block. What do you think about my code this time? I truly appreciate your enlightenment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure that blogger images are never inline? That seems hard to believe. Do you have some source for this info?

Copy link
Author

Choose a reason for hiding this comment

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

Yah, you're right. I'm not completely sure the images from blogger are never inline. Hmm...

@phast184 phast184 requested a review from humphd November 18, 2020 16:02
humphd
humphd requested changes Nov 20, 2020
View reviewed changes
Copy link
Contributor

@humphd humphd 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 the only change we should be making is to remove the width: 100%. The rest of this seems wrong to me.

@yuanLeeMidori
Copy link
Contributor

@phast184 Hi! Telescope team is working on merging the open PRs. Just wondering do you still have interest working on this PR and get merged?
If you want to continue working on it, you might need to check this first: #1444.

Please leave a comment if you decide to drop this PR, we'll assign another team member to work on it. Thank you :)

@humphd humphd added this to the 1.6 Release milestone Jan 19, 2021
@humphd
Copy link
Contributor

humphd commented Jan 19, 2021

This is going to need to be rebased. Also, let's get the next.js port finished before we tackle this, and then backport from Gatsby to Next.js.

@PedroFonsecaDEV PedroFonsecaDEV added type: bug Something isn't working Priority: High and removed Priority: High labels Jan 26, 2021
@birtony birtony modified the milestones: 1.6 Release, 1.7 Release Jan 26, 2021
@birtony birtony added area: css styling Anything related to CSS styling area: front-end labels Jan 26, 2021
@birtony birtony added area: nextjs Nextjs related issues Priority: Medium labels Jan 26, 2021
@birtony
Copy link
Contributor

birtony commented Jan 26, 2021

As agreed during our triage meeting, we will postpone work on this bug until we finish porting front-end to Next.js.

@humphd
Copy link
Contributor

humphd commented Feb 2, 2021

See also #1412, which is related to this.

@huyxgit huyxgit modified the milestones: 1.7 Release, 1.8 Release Feb 16, 2021
@yuanLeeMidori yuanLeeMidori mentioned this pull request Feb 18, 2021
Merged
8 tasks
@yuanLeeMidori
Copy link
Contributor

closed by #1783

@yuanLeeMidori yuanLeeMidori mentioned this pull request Feb 19, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@humphd humphd humphd requested changes

Assignees

@yuanLeeMidori yuanLeeMidori

@Metropass Metropass

Labels
area: css styling Anything related to CSS styling area: front-end area: nextjs Nextjs related issues type: bug Something isn't working
Projects
None yet
Milestone
1.8 Release
Development

Successfully merging this pull request may close these issues.
7 participants
@phast184 @humphd @yuanLeeMidori @birtony @huyxgit @Metropass @PedroFonsecaDEV