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

Switch help modals from angular templates to use ViewComponent and StimulusJs #9040

Merged
merged 1 commit into from
Apr 20, 2022

Conversation

cillian
Copy link
Contributor

@cillian cillian commented Mar 25, 2022

What? Why?

Partially addresses #8700

We need to get to a place where we can remove the unsafe_eval directive from our CSP configuration in config/initializers/content_security_policy.rb. In order to do that we need to remove/replace a few things in the codebase (otherwise the app will be broken). The main one is our use of Angular Templates.

Before:

help-modals-before

After:

help-modals-after

I'm not sure the best way to do the CSS for this so I copied #8996 and moved it into the component folder and reused the login modal .reveal-modal classes.

What should we test?

The following modals have been updated to use StimulusJS instead of angular templates.

  1. Enterprise settings > Business details > Terms & Conditions
  2. Enterprise settings > Business details > Legal company name
  3. Enterprise settings > Business details > Legal name
  4. Enterprise settings > Business details > Legal phone number
  5. Enterprise settings > Payment methods > Stripe connect
  6. Enterprise settings > Tag rules
  7. Enterprise settings > Users > Add an unregistered user
  8. Account > Credit cards > Saved cards (As logged-in customer)

Release notes

Changelog Category: Technical changes

@cillian cillian self-assigned this Mar 25, 2022
Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

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

Wow, thank you!

Copy link
Contributor

@jibees jibees left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@filipefurtad0
Copy link
Contributor

Hey @cillian ,
Some conflicts arose in the meantime - could you have a quick re-base on this one? Thank you 🙏

@cillian
Copy link
Contributor Author

cillian commented Apr 15, 2022

Hi @filipefurtad0

I forced pushed to fix that conflict now, it was...

<<<<<<< HEAD
.row
  %fieldset.alpha.no-border-bottom
    %legend= t('.Invoice_item_sorting')
  .three.columns.alpha
    %label= t('.sort_items_by_supplier?')
    %div{'ofn-with-tip' => t('.sort_items_by_supplier_tip')}
      %a= t 'admin.whats_this'
  .three.columns
    = f.radio_button :preferred_invoice_order_by_supplier, true, 'ng-model' => 'Enterprise.preferred_invoice_order_by_supplier', 'ng-value' => 'true'
    = f.label :preffered_invoice_order_by_supplier, t('.enabled'), value: :true
  .five.columns.omega
    = f.radio_button :preferred_invoice_order_by_supplier, false, 'ng-model' => 'Enterprise.preferred_invoice_order_by_supplier', 'ng-value' => 'false'
    = f.label :preferred_invoice_order_by_name, t('.disabled'), value: :false
=======
= render HelpModalComponent.new(id: "terms_and_conditions_info_modal") do
  .margin-bottom-30.text-center
    .text-big
      = t('js.admin.modals.terms_and_conditions_info.title')
  .margin-bottom-30
    %p
      = t('js.admin.modals.terms_and_conditions_info.message_1')
  .margin-bottom-30
    %p
      = t('js.admin.modals.terms_and_conditions_info.message_2')
>>>>>>> 78d13912e (Switch help modals from angular templates to use ViewComponent and StimulusJs)

There is one new Rubocop error here which says to add a call to super. I didn't fix that if that's okay with you because I don't think it is needed in ViewComponent classes, see the example here?

@jibees
Copy link
Contributor

jibees commented Apr 15, 2022

There is one new Rubocop error here which says to add a call to super. I didn't fix that if that's okay with you because I don't think it is needed in ViewComponent classes, see the example here?

Yep, this is not needed I guess. We probably should get rid of this warning. Will address this in a PR (done, #9104)

@filipefurtad0 filipefurtad0 added pr-staged-uk staging.openfoodnetwork.org.uk and removed feedback-needed pr-staged-uk staging.openfoodnetwork.org.uk labels Apr 15, 2022
@filipefurtad0
Copy link
Contributor

filipefurtad0 commented Apr 19, 2022

Hey @cillian ,

For some reason, I was unable to stage this with Semaphore... And while trying to figure out why, other merge conflicts came up.
So I tried to fix these, without force pushing into your branch, and the result was that all those other commits got added to this PR...

...but, the merge conflicts were solved 😄 and we could deploy this manually (ansiable) without recurring to semaphore. This allowed to test the PR - notes on the next message 👍

All tests are green, but I'm 100% sure if it is ok to merge in this state, so a quick reply with your agreement or from another dev would be reassuring - ping @mkllnk

Below, the testing notes. Thanks again for this PR 🎉

@filipefurtad0
Copy link
Contributor

filipefurtad0 commented Apr 19, 2022

Following your detailed "What to test" section - the following modals have been tested:

Left - staging FR (master); right staging-UK (this PR)

  • Enterprise settings > Business details > Terms & Conditions
  • Enterprise settings > Business details > Legal company name
  • Enterprise settings > Business details > Legal name
  • Enterprise settings > Business details > Legal phone number

Peek 2022-04-19 17-54

  • Enterprise settings > Tag rules
    image

  • Enterprise settings > Users > Add an unregistered user
    image

  • Account > Credit cards > Saved cards (As logged-in customer)
    image

  • Enterprise settings > Payment methods > Stripe connect
    

Peek 2022-04-19 17-51

@filipefurtad0 filipefurtad0 removed the pr-staged-uk staging.openfoodnetwork.org.uk label Apr 19, 2022
@mkllnk mkllnk force-pushed the stimulus-js-help-modals branch from ad1e9b3 to 4162830 Compare April 19, 2022 23:36
@mkllnk mkllnk merged commit 67d3bd7 into openfoodfoundation:master Apr 20, 2022
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.

4 participants