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/issue254 255 modal #257

Merged
merged 11 commits into from
Nov 7, 2017
Merged

Fix/issue254 255 modal #257

merged 11 commits into from
Nov 7, 2017

Conversation

ibelar
Copy link
Contributor

@ibelar ibelar commented Nov 1, 2017

Fix issue 254

  • Apply loading indicator to modal create either via jsModal or Modal.php
  • allow to supply loading indicator label, default to "Loading...", from jsModal or Modal

Enhancement issue 255

  • Allow to pass url argument to Modal::show($arg) method.
$app->add(['Button', 'Show'])->on('click', $modal->show(['color'=>'blue']));

Apply some refactoring on passing data to modal

  • data is now pass via the $.data() function either for Modal.php or jsModal, thus removing the html data attribute in modal.html template.

Note: Atk4JS need rebuilding.

ibelar and others added 2 commits November 1, 2017 16:17
add loader to modal create via Modal.php
allow url argument to be pass in Modal::show([‘color’=>’blue’])
pass modal data via jQuery $.data method instead of html data attribute.
simply data management between modal create via jsModal or Modal.php
@codecov
Copy link

codecov bot commented Nov 1, 2017

Codecov Report

Merging #257 into develop will increase coverage by <.01%.
The diff coverage is 81.81%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop     #257      +/-   ##
=============================================
+ Coverage      72.12%   72.13%   +<.01%     
- Complexity      1111     1113       +2     
=============================================
  Files             62       62              
  Lines           2730     2738       +8     
=============================================
+ Hits            1969     1975       +6     
- Misses           761      763       +2
Impacted Files Coverage Δ Complexity Δ
src/Modal.php 42.68% <81.81%> (+3.49%) 32 <2> (+2) ⬆️

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 1c9dc8a...0340e06. Read the comment docs.

@romaninsh
Copy link
Member

Is this ready for review?

@romaninsh
Copy link
Member

I tried added test, but I'm getting a failure. Maybe I'm not doing it correctly?

screen shot 2017-11-02 at 11 55 14

ibelar and others added 3 commits November 2, 2017 08:22
Change how url query are set using Modal::Show();
Now return with js chain instead of waiting for modal to render.
This way will work when loading a modal from other virtual page like
form on success.
@ibelar
Copy link
Contributor Author

ibelar commented Nov 2, 2017

Change the way how arguments are save to modal when using Modal::show($args).

Also remove view tester on second modal since tester will reset Message color on it's last callback because $_Get['color'] is not available when view tester reload.

@romaninsh
Copy link
Member

works perfectly now. Will merge now.

@romaninsh romaninsh merged commit 92500f4 into develop Nov 7, 2017
@romaninsh romaninsh deleted the fix/issue254-255-modal branch November 7, 2017 22:56
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.

2 participants