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: Issues with Modal and Image Cropper position #5367

Conversation

sachinchauhan2889
Copy link
Contributor

@sachinchauhan2889 sachinchauhan2889 commented Oct 23, 2020

Fixes #4837, #4917, #4716, #4505, #5308

Short description of what this resolves:

this PR can solve issue #5308 #4917 #4837 #4716 #4505 .

changes

I have tested all modals and croppers mentioned above from all scroll positions and get modal at center.

Checklist

  • I have read the Contribution & Best practices Guide.
  • My branch is up-to-date with the Upstream development branch.
  • The acceptance, integration, unit tests and linter pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

screenshots

#4505
final 4505

#4716
final 4716

#4837 #5308
final 4837 5308

#4917
final 4917

@vercel
Copy link

vercel bot commented Oct 23, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/eventyay/open-event-frontend/jwdz7zmdy
✅ Preview: https://open-event-frontend-git-align-sessions-notify-modal-4837.eventyay.now.sh

if (this.noCropper) {
$element.modal({
centered: false
}).modal('show');
Copy link
Member

Choose a reason for hiding this comment

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

This will ignore all the settings done in the modal below. As evident by the dark dimmer in the modal. Make this change in the settings below

@@ -16,6 +16,7 @@ export default class SessionNotifyModal extends ModalBase {

constructor() {
super(...arguments);
this.noCropper = true;
Copy link
Member

Choose a reason for hiding this comment

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

Pass this as a prop rather than setting here

@sachinchauhan2889
Copy link
Contributor Author

@iamareebjamal thank you for your reviews. i am working on changes suggested by you.

@iamareebjamal
Copy link
Member

Like my review explained why changing it this way is not good enough, @mariobehling has said

Thank you!

  • Could you keep the overlay background semitransparent white, please for consistency?
  • Please make the X button black (on white background).
  • When a user closes the pop up using the X button and then clicks add tax information again nothing happens. Expected: It should open the pop up again.

@sachinchauhan2889
Copy link
Contributor Author

This will ignore all the settings done in the modal below. As evident by the dark dimmer in the modal. Make this change in the settings below

@iamareebjamal i try to make changes to settings in willInitSemantic(settings){...} function.
But centered property of modal not assigned with false. I think Settings of semantic ui modal not have any property named centered. https://semantic-ui.com/modules/modal.html#/settings

@sachinchauhan2889
Copy link
Contributor Author

@iamareebjamal can i assign all properties here.

$element.modal({
centered: false
}).

@iamareebjamal
Copy link
Member

It should have the same interface of settings, but if it is not working, you can try that

@sachinchauhan2889
Copy link
Contributor Author

@iamareebjamal please have a look at this.
Now this PR can solve issue #5308 #4917 #4837 #4716 #4505 .

changes

I have tested all modals and croppers mentioned above from all scroll positions and get modal at center.

screenshots

#4505
final 4505

#4716
final 4716

#4837 #5308
final 4837 5308

#4917
final 4917

@sachinchauhan2889 sachinchauhan2889 changed the title fix: Organizer Session Notifications: Message Box if Off-Screen fix: Issues with Modal and image Cropper position Oct 24, 2020
fix-all-modals-crpper

chore(deps-dev): bump @sentry/tracing from 5.27.0 to 5.27.1 (fossasia#5372)

Bumps [@sentry/tracing](https://github.com/getsentry/sentry-javascript) from 5.27.0 to 5.27.1.
- [Release notes](https://github.com/getsentry/sentry-javascript/releases)
- [Changelog](https://github.com/getsentry/sentry-javascript/blob/master/CHANGELOG.md)
- [Commits](getsentry/sentry-javascript@5.27.0...5.27.1)

Signed-off-by: dependabot-preview[bot] <support@dependabot.com>

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>

chore(deps-dev): bump mini-css-extract-plugin from 1.1.2 to 1.2.0 (fossasia#5371)

Bumps [mini-css-extract-plugin](https://github.com/webpack-contrib/mini-css-extract-plugin) from 1.1.2 to 1.2.0.
- [Release notes](https://github.com/webpack-contrib/mini-css-extract-plugin/releases)
- [Changelog](https://github.com/webpack-contrib/mini-css-extract-plugin/blob/master/CHANGELOG.md)
- [Commits](webpack-contrib/mini-css-extract-plugin@v1.1.2...v1.2.0)

Signed-off-by: dependabot-preview[bot] <support@dependabot.com>

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
classNameBindings : ['isFullScreen:fullscreen', 'isSmall:small', 'isLarge:large'],

openObserver: observer('isOpen', function() {
const $element = $(this.element);
if (this.isOpen) {
$element.modal('show');
$element.modal({
Copy link
Member

Choose a reason for hiding this comment

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

Copied code. Move to a single function

@@ -91,7 +129,44 @@ export default UiModal.extend({

didInitSemantic() {
if (this.isOpen) {
$(this.element).modal('show');
$(this.element).modal({
Copy link
Member

Choose a reason for hiding this comment

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

Duplicate code. Remove any duplicate code

@@ -1,3 +1,4 @@
<i class="black close icon"></i>
Copy link
Member

Choose a reason for hiding this comment

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

There is already a closable icon in the modal I believe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iamareebjamal not present earlier. please see #4505

@sachinchauhan2889
Copy link
Contributor Author

@iamareebjamal please have a look at this PR. i have removed Duplicacy of code.

classNameBindings : ['isFullScreen:fullscreen', 'isSmall:small', 'isLarge:large'],

openObserver: observer('isOpen', function() {
const $element = $(this.element);
if (this.isOpen) {
$element.modal('show');
$element.modal({...this.defaultOptions}).modal('show');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$element.modal({...this.defaultOptions}).modal('show');
$element.modal(this.defaultOptions).modal('show');

assign(settings, options);
},

didInitSemantic() {
if (this.isOpen) {
$(this.element).modal('show');
$(this.element).modal({...this.defaultOptions}).modal('show');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$(this.element).modal({...this.defaultOptions}).modal('show');
$(this.element).modal(this.defaultOptions).modal('show');

@iamareebjamal
Copy link
Member

Since now you're only using the default options of the modal, what's the need for passing them in the modal(this.defaultOptions) call? Try removing them and try again, it should work without passing the settings

@sachinchauhan2889
Copy link
Contributor Author

Since now you're only using the default options of the modal, what's the need for passing them in the modal(this.defaultOptions) call? Try removing them and try again, it should work without passing the settings

@iamareebjamal sir i have tried this but modal is not appearing at center. All properties are present in settiings of modal but centered: false is not present in settings. For this centered: false property i have used defaultOptions object and pass it here.

I think this is because the settings of semantic ui modal not contain centered as property. This is why i have make defaultOptions object in init() function and use this object in $(this.element).modal(defaultOptions).modal('show').

$(this.element).modal('show') is not working.

i have done all changes suggested by you. have a look at this.

@iamareebjamal
Copy link
Member

Checking

@iamareebjamal
Copy link
Member

iamareebjamal commented Oct 24, 2020

Good job. But there are still some issues. I am mentioning them one by one.

#4837: If you decrease the height of the screen a little bit, the modal shifts to right
image

Test on heights < 825px

Same issue in #4917, #4716, #4505

@iamareebjamal
Copy link
Member

I think your styles are being overriden in a media query

@iamareebjamal
Copy link
Member

When the screen is smaller in height than 825px, semantic adds left: 50% which shifts the dialog further to the right. You need to add left:auto !important in your CSS style to make sure it stays in center at any height

@codecov
Copy link

codecov bot commented Oct 24, 2020

Codecov Report

Merging #5367 into development will decrease coverage by 0.05%.
The diff coverage is 50.00%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #5367      +/-   ##
===============================================
- Coverage        22.78%   22.72%   -0.06%     
===============================================
  Files              491      491              
  Lines             5245     5245              
  Branches            37       37              
===============================================
- Hits              1195     1192       -3     
- Misses            4045     4048       +3     
  Partials             5        5              
Impacted Files Coverage Δ
app/components/modals/tax-info-modal.js 22.22% <ø> (ø)
app/components/modals/modal-base.js 25.00% <50.00%> (ø)
app/models/event.js 31.25% <0.00%> (-18.75%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 03422cf...6c5946c. Read the comment docs.

@iamareebjamal iamareebjamal changed the title fix: Issues with Modal and image Cropper position fix: Issues with Modal and Image Cropper position Oct 24, 2020
@iamareebjamal iamareebjamal merged commit 7971c4a into fossasia:development Oct 24, 2020
@iamareebjamal
Copy link
Member

Thanks a lot for solving a complex and long standing issue with little code changes elegantly

@sachinchauhan2889
Copy link
Contributor Author

Thanks a lot for solving a complex and long standing issue with little code changes elegantly

@iamareebjamal my pleasure sir

@sachinchauhan2889 sachinchauhan2889 deleted the align-sessions-notify-modal-#4837 branch October 26, 2020 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment