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

Remove SWAL library #7018

Merged
merged 9 commits into from
Dec 21, 2022
Merged

Remove SWAL library #7018

merged 9 commits into from
Dec 21, 2022

Conversation

haoweiqiu
Copy link
Contributor

Brief summary of changes

Try to remove SWAL library.

Link(s) to related issue(s)

@haoweiqiu haoweiqiu requested a review from laemtl September 8, 2020 18:36
@haoweiqiu haoweiqiu added the Cleanup PR or issue introducing/requiring at least one clean-up operation label Sep 8, 2020
@christinerogers christinerogers added the State: Needs work PR awaiting additional work by the author to proceed label Sep 11, 2020
@christinerogers
Copy link
Contributor

Hi Laetitia @laemtl and Shen @kongtiaowang - Any ideas here for the integration test failures we're seeing in Travis?

@laemtl
Copy link
Contributor

laemtl commented Sep 11, 2020

@christinerogers I investigated a few things with @haoweiqiu and they passed. Closing to see if we get the same result twice. Otherwise, my understanding is that we have to investigate each case individually to discover what's wrong.

@laemtl laemtl closed this Sep 11, 2020
@laemtl laemtl reopened this Sep 11, 2020
@laemtl laemtl closed this Sep 17, 2020
@laemtl laemtl reopened this Sep 17, 2020
@haoweiqiu haoweiqiu marked this pull request as ready for review September 17, 2020 22:06
@laemtl laemtl removed the State: Needs work PR awaiting additional work by the author to proceed label Sep 18, 2020
@laemtl
Copy link
Contributor

laemtl commented Sep 18, 2020

Made a small change and Travis is passing now.

@laemtl
Copy link
Contributor

laemtl commented Sep 18, 2020

Find one last occurence that may be problematic in htdocs/js/instrument_controlpanel_swaldeletedata.js:

function swalFunction(event) {
  swal.fire({
    title: 'Please confirm deletion',
    text: 'The instrument data will be deleted',

referenced in smarty/templates/instrumentstatus_controlpanel.tpl.

@haoweiqiu
Copy link
Contributor Author

Hi @laemtl I have also found this function. It was originally swal, but I changed to swal.fire (Sweetalert 2).

@laemtl
Copy link
Contributor

laemtl commented Sep 18, 2020

@haoweiqiu It is not embedded in a react component so this change will not work. @maltheism, any suggestions?

@laemtl
Copy link
Contributor

laemtl commented Sep 19, 2020

Fixed by reactifying the component (htdocs/js/instrument_controlpanel_swaldeletedata.js was migrated in modules/instruments/jsx/ControlpanelDeleteInstrumentData.js)

@christinerogers
Copy link
Contributor

Hi @laemtl - could you comment on how this PR should move forward?
Is it still relevant - does it just need @haoweiqiu to rebase it?

@laemtl
Copy link
Contributor

laemtl commented Nov 27, 2020

This PR is ready for review once it is rebased. I think it is still relevant, except if @ridz1208 or @driusan think otherwise.

@laemtl laemtl closed this Mar 11, 2021
@laemtl laemtl reopened this Mar 11, 2021
@laemtl laemtl added the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label Mar 11, 2021
@christinerogers
Copy link
Contributor

@haoweiqiu would you be able to rebase this? Thank you -

@zaliqarosli
Copy link
Contributor

zaliqarosli commented Jul 19, 2021

@laemtl @ridz1208 @christinerogers guys i think we should probably keep this library until all modules are reactified? sweetalert2 only works if embedded in a react component.

i would like to use sweetalert in this PR https://github.com/aces/Loris/pull/7069/files targeting the non-reactified configuration module. how should i move forward? in my mind, i would like to keep our UI consistent across modules

@cmadjar
Copy link
Collaborator

cmadjar commented Aug 10, 2021

@driusan to be discussed at the LORIS meeting.

@cmadjar cmadjar assigned maltheism and unassigned kongtiaowang Nov 29, 2022
@laemtl
Copy link
Contributor

laemtl commented Nov 29, 2022

@maltheism This PR has been updated to address @zaliqarosli comment. It should be ready to go after a rebase.

@laemtl laemtl added the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label Nov 29, 2022
@maltheism maltheism removed the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label Dec 2, 2022
@maltheism maltheism added the Passed manual tests PR has been successfully tested by at least one peer label Dec 2, 2022
@driusan
Copy link
Collaborator

driusan commented Dec 6, 2022

@maltheism can you rebase again? I think I merged a different one of your PRs today that added a reference to it somewhere else, I want to be sure the tests still pass..

@maltheism maltheism force-pushed the 2020-09-08-Remove_swal branch from 8d0890a to 37fe69a Compare December 6, 2022 20:39
@maltheism
Copy link
Member

@driusan I think this PR makes my other PR irrelevant. I manually tested the preview button of the help_editor module and it works correctly in this PR as well.

@driusan
Copy link
Collaborator

driusan commented Dec 6, 2022

@maltheism conflicts in .eslintrc.json too

@zaliqarosli zaliqarosli linked an issue Dec 20, 2022 that may be closed by this pull request
@driusan driusan merged commit 77e403f into aces:main Dec 21, 2022
driusan pushed a commit that referenced this pull request Dec 22, 2022
Add formData to fetch.

Fixes #6940 regression in that broke in #7018
@ridz1208 ridz1208 added this to the 25.0.0 milestone Mar 6, 2023
driusan pushed a commit that referenced this pull request May 11, 2023
Fix clear instrument button regression introduced by #7018 and reported in PR #8673 

Fixes #7065, Fixes #8580
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cleanup PR or issue introducing/requiring at least one clean-up operation Passed manual tests PR has been successfully tested by at least one peer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[HelpEditor] Not saving changes Remove SWAL from htdocs/vendor
10 participants