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

Zoom in and out image #1448 #2809

Merged
merged 7 commits into from
Jan 28, 2019
Merged

Zoom in and out image #1448 #2809

merged 7 commits into from
Jan 28, 2019

Conversation

roottool
Copy link
Contributor

Description

I added a new feature like the Medium Image Zoom feature.
zoominoutimage

Issue fixed

#1448

Type of changes

  • ⚪ Bug fix (Change that fixed an issue)
  • ⚪ Breaking change (Change that can cause existing functionality to change)
  • ⚪ Improvement (Change that improves the code. Maybe performance or development improvement)
  • 🔘 Feature (Change that adds new functionality)
  • ⚪ Documentation change (Change that modifies documentation. Maybe typo fixes)

Checklist:

  • 🔘 My code follows the project code style
  • ⚪ I have written test for my code and it has been tested
  • 🔘 All existing tests have been passed
  • 🔘 I have attached a screenshot/video to visualize my change if possible

Note

I was able to test all existing tests by Windows subsystem for linux(Ubuntu 18.04).

@ZeroX-DG ZeroX-DG added the awaiting review ❇️ Pull request is awaiting a review. label Jan 16, 2019
@ZeroX-DG
Copy link
Member

Nice feature!

@MiloTodt
Copy link
Contributor

Great feature!

@@ -832,6 +832,82 @@ export default class MarkdownPreview extends React.Component {
)
}
)

document.querySelectorAll('iframe').forEach(item => {
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need to select all iframes like this? The markdown preview only contains 1 iframe, then why did you have to find all iframes and loop through them? Can you explain this to me?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Explanation

The code is my mistake.
Because when I tried to catch image elements into iframe, I used the code as it is without thinking.

Suggestion

I have a suggestion to resolve it.
suggestion
document.querySelector() searches for an element from entire HTML.
I considered the possibility of adding new iframes in Boostnote.
So I use a classname of a markdown preview iframe.

It has a pros and cons.
Pros

  • Specifying an element is easy because MarkdownPreview used only here.

Cons

  • If a classname changes from MarkdownPreview, a classname using document.querySelector() will need to change too.

Before

    document.querySelectorAll('iframe').forEach(item => {
      const rect = item.getBoundingClientRect()
      const imgList = item.contentWindow.document.body.querySelectorAll('img')

After

    const markdownPreviewIframe = document.querySelector('.MarkdownPreview')
    const rect = markdownPreviewIframe.getBoundingClientRect()
    const imgList = markdownPreviewIframe.contentWindow.document.body.querySelectorAll('img')

Copy link
Member

Choose a reason for hiding this comment

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

We can just go for the 2nd method, I don't think there's any reason to change the MarkdownPreview class name. In case we were going to change it, a simple search & replace for the MarkdownPreview class wouldn't be hard.

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 was worried about changing only in HTML.
I want to modify from before code to after code because I feel relieved by your reply.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, you can just go with the after code 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I rewrote to after code.
Please check the code again.


const zoomImgWidth = img.width * magnification
const zoomImgHeight = img.height * magnification
const zoomImgTop = document.body.clientHeight / 2 - zoomImgHeight / 2
Copy link
Member

Choose a reason for hiding this comment

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

Little suggestion, you can transform this code into:

const zoomImgTop = (document.body.clientHeight - zoomImgHeight) / 2

This way, we only need to do 2 calculation instead of 3

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 didn't notice it.
I am going to fix it tomorrow.
Thank you for teaching it!

Copy link
Member

@ZeroX-DG ZeroX-DG Jan 24, 2019

Choose a reason for hiding this comment

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

No problem 👍 This is a great feature!

@ZeroX-DG ZeroX-DG added awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. and removed awaiting review ❇️ Pull request is awaiting a review. labels Jan 22, 2019
Copy link
Member

@ZeroX-DG ZeroX-DG left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@ZeroX-DG ZeroX-DG added approved 👍 Pull request has been approved by sufficient reviewers. and removed awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. labels Jan 27, 2019
@Rokt33r Rokt33r added this to the v0.11.14 milestone Jan 28, 2019
@Rokt33r Rokt33r merged commit 950d31a into BoostIO:master Jan 28, 2019
@roottool roottool deleted the zoom-in-and-out-image#1448 branch January 29, 2019 05:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved 👍 Pull request has been approved by sufficient reviewers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants