Skip to content

Conversation

divyamtayal
Copy link
Member

@divyamtayal divyamtayal commented Nov 11, 2020

Fixes #5526

Short description of what this resolves:

Fixed showing empty email fields in the team.

Changes proposed in this pull request:

  • when we click on add people and then click on side of the screen, the empty entry that was created for creating the people still appears which is fixed by adding an if statement

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)

@vercel
Copy link

vercel bot commented Nov 11, 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/nirgh3cf3
✅ Preview: https://open-event-frontend-git-empty-email-5526.eventyay.vercel.app

@codecov
Copy link

codecov bot commented Nov 11, 2020

Codecov Report

Merging #5584 (77048ff) into development (ff1d83c) will increase coverage by 0.05%.
The diff coverage is 0.00%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #5584      +/-   ##
===============================================
+ Coverage        23.70%   23.75%   +0.05%     
===============================================
  Files              498      498              
  Lines             5257     5258       +1     
  Branches            44       44              
===============================================
+ Hits              1246     1249       +3     
+ Misses            4005     4003       -2     
  Partials             6        6              
Impacted Files Coverage Δ
app/components/modals/modal-base.js 24.24% <0.00%> (-0.76%) ⬇️
app/components/tabbed-navigation.js 53.33% <0.00%> (+20.00%) ⬆️

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 ff1d83c...77048ff. Read the comment docs.

@divyamtayal
Copy link
Member Author

@iamareebjamal Please see to the changes I have made.
These changes were made only in UI bcz the empty entries on reloading page is no longer shown but only shown when being on same page and clicking add people and then clicking on backdrop.

@iamareebjamal
Copy link
Member

That's not a fix. The entry should be deleted if someone clicks out of the window

@divyamtayal
Copy link
Member Author

@iamareebjamal Please see the changes I have made and let me know to change anything.

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

Choose a reason for hiding this comment

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

Please remove. You already have a callback for modal closing. Makes no sense to put logic of user role modal in modal base

Copy link
Member Author

@divyamtayal divyamtayal Nov 12, 2020

Choose a reason for hiding this comment

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

@iamareebjamal then the solution to this problem could be disabling clicking outside the window so that the modal won't close unless user click on cancel button and clicking on cancel button is already deleting the empty record created.

Copy link
Member

Choose a reason for hiding this comment

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

Not really. There are tons of ways to solve it

Copy link
Member Author

Choose a reason for hiding this comment

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

@iamareebjamal So in the previous solution where I was calling the function when the user click outside the box in the modal-base.Is this not be good enough sol? bcz what actually we want to do is that calling the function when the modal closes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Please remove. You already have a callback for modal closing. Makes no sense to put logic of user role modal in modal base

@iamareebjamal and I have removed the function which was calling for modal closing and put it inside modal-base bcz we want same thing to happen even if user click on cancel button or outside the model

@divyamtayal
Copy link
Member Author

@iamareebjamal I have come with the changes. Please see to the changes I have made now and let me know to change anything

@iamareebjamal
Copy link
Member

Check every other modal that it's working correctly

@divyamtayal
Copy link
Member Author

Check every other modal that it's working correctly

@iamareebjamal I have checked all the modals and every modal is working correctly.

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

Choose a reason for hiding this comment

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

Won't this cause recursive error?

Copy link
Member Author

Choose a reason for hiding this comment

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

No its no causing

@iamareebjamal iamareebjamal changed the title Fixed: Team Page Empty email fields fix: Team Page Empty email fields Nov 12, 2020
@iamareebjamal iamareebjamal merged commit 8719978 into fossasia:development Nov 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Team Page : Empty email fields

2 participants