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

Image fullscreen preview #181

Merged
merged 7 commits into from
May 29, 2020
Merged

Image fullscreen preview #181

merged 7 commits into from
May 29, 2020

Conversation

Here21
Copy link
Contributor

@Here21 Here21 commented Apr 29, 2020

fullscreen

Features

  • fullscreen preview;
  • mobile support;

New "imagePreview" Widget props support message image preview

@Here21 Here21 changed the title Dev/full screen Image fullscreen preview Apr 29, 2020
Copy link
Contributor

@mcallegari10 mcallegari10 left a comment

Choose a reason for hiding this comment

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

Amazing work! Just a couple of comments for standards and some questions 🎉 😄

width: naturalWidth,
height: naturalHeight,
};
dispatch(openFullscreenPreview(obj))
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if I'm on board with the Messages Container to be the one dispatching the full screen behavior. Why are you doing it in this component instead of creating some Image component? Though I really like the markdown approach, I think there could be a way to combine both. What do you think? 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean to add a method like "addLinkSnippet"?
I think markdown syntax is more convenient and does not need to write too much "Snippet", we only need to write once.
What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, something like that addImage method, that should be like the Link Snippet, is what I had in mind. But of course the markdown solution is really clever and I really like it, my only issue with it is we are doing a lot of stuff in the Container and I wanted to decouple it a little. We can leave it to another version though 😄

src/store/dispatcher.ts Outdated Show resolved Hide resolved
Copy link
Contributor Author

@Here21 Here21 left a comment

Choose a reason for hiding this comment

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

@mcallegari10 Could you please review my code

width: naturalWidth,
height: naturalHeight,
};
dispatch(openFullscreenPreview(obj))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean to add a method like "addLinkSnippet"?
I think markdown syntax is more convenient and does not need to write too much "Snippet", we only need to write once.
What do you think?

})
}
>
{showChat &&
Copy link
Contributor Author

Choose a reason for hiding this comment

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

bug fixed here.
but animation is not working.
I am looking for the solution

Copy link
Contributor

Choose a reason for hiding this comment

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

Was the issue about the render of the component? The animation can be made via CSS and control the render of the component via state variables 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, I do this already. I try to use JS to control DOM, but it's not best solution for performance. So we can leave it to another version though ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Of course, animations are pretty rough so those are something to work on 😄

dev/App.tsx Outdated
@@ -29,14 +29,17 @@ export default class App extends Component {

render() {
return (
<div>
<button style={{position: 'absolute', right: 40, bottom: 150}}>test</button>
<Widget
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There has a bug.
bug
fixed at src/components/Widget/layout.tsx

@Here21
Copy link
Contributor Author

Here21 commented May 9, 2020

@mcallegari10 Hi, would you have suggestion for this?

Copy link
Contributor

@mcallegari10 mcallegari10 left a comment

Choose a reason for hiding this comment

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

Just a couple of comments and suggestions. Sorry for the delay! 🙏

Comment on lines 105 to 109
if(fullScreenMode) {
document.body.setAttribute('style', "overflow: hidden")
} else {
document.body.setAttribute('style', "overflow: auto")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You can do the same here:

document.body.setAttribute('style', `overflow: ${fullScreenMode ? 'hidden' : 'auto'}`);

})
}
>
{showChat &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Was the issue about the render of the component? The animation can be made via CSS and control the render of the component via state variables 🤔

width: naturalWidth,
height: naturalHeight,
};
dispatch(openFullscreenPreview(obj))
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, something like that addImage method, that should be like the Link Snippet, is what I had in mind. But of course the markdown solution is really clever and I really like it, my only issue with it is we are doing a lot of stuff in the Container and I wanted to decouple it a little. We can leave it to another version though 😄

@Here21
Copy link
Contributor Author

Here21 commented May 17, 2020

Sorry for the delay, last week so busy.😅

Copy link
Contributor

@mcallegari10 mcallegari10 left a comment

Choose a reason for hiding this comment

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

Again, amazing job! Thanks for your contribution 😄 💪

@mcallegari10 mcallegari10 merged commit 28ffcb1 into Wolox:master May 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants