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

Replace Deface Override with Deface DSL #64

Conversation

sascha-karnatz
Copy link
Contributor

The override objects don't work great in a combination of Rails 7.1 and Zeitwerk. Also the html.erb.deface can contain the partial itself and it is possible to remove the additional partials.

One of the overrides had a typo (insert_bottom: "ul.tollbar") and it is most likely that it didn't worked for the last 8 years. It was also not possible to find the selectors for '[data-hook="new_product_tax_category"]', and '[data-hook="new_product_price"]'. Maybe it is possible to remove these defaces if they not in use anymore.

@tvdeyen
Copy link
Member

tvdeyen commented Apr 15, 2024

🤣 The tollbar class actually exists in https://github.com/solidusio/solidus/blob/main/backend/app/views/spree/admin/product_properties/index.html.erb#L8

@tvdeyen
Copy link
Member

tvdeyen commented Apr 15, 2024

@sascha-karnatz spec failure seems to be legit

@sascha-karnatz sascha-karnatz force-pushed the replace-deface-override-with-dsl branch 2 times, most recently from 2367d1a to 7c7bb08 Compare April 16, 2024 14:40
@tvdeyen
Copy link
Member

tvdeyen commented Apr 17, 2024

Specs started to fail with Rack 3.0 because of how nested params are handled in https://github.com/solidusio-contrib/solidus_prototypes/blob/main/app/views/spree/admin/prototypes/show.html.erb#L14

See rack/rack#2128

Possible solutions:

  1. Try to fix the params (unlikely, without a change in Solidus Core)
  2. Stick below Rack 3 (not nice, but working)

This was referenced Apr 17, 2024
The override objects don't work great in a combination of Rails 7.1 and Zeitwerk. Also the html.erb.deface can contain the partial itself and it is possible to remove the additional partials.
@tvdeyen tvdeyen force-pushed the replace-deface-override-with-dsl branch from 7c7bb08 to 3ed7ba5 Compare April 17, 2024 08:03
@tvdeyen
Copy link
Member

tvdeyen commented Apr 17, 2024

Rebased with latest main to fix specs (because of #65)

@tvdeyen tvdeyen merged commit 0901a15 into solidusio-contrib:main Apr 17, 2024
5 checks passed
@sascha-karnatz sascha-karnatz deleted the replace-deface-override-with-dsl branch April 17, 2024 08:44
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.

None yet

3 participants