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

De-deface some admin configuration pages and the general admin layout and menu #4047

Merged
merged 37 commits into from
Jul 23, 2019

Conversation

luisramos0
Copy link
Contributor

@luisramos0 luisramos0 commented Jul 16, 2019

What? Why?

There's no issue for this but this is part of #2807. In this PR I am de-defacing a few things in the backoffice:

  • configurations - general settings
  • configurations - image settings
  • configurations - shipping categories
  • main admin layout and admin/shared/head
  • main admin menu

No functional changes done, just bringing code from spree, converting to haml and applying the defaces.

What should we test?

Make sure these pages show all fields and work as before:

  • Configurations - General Settings page
  • Configurations - Image Settings
  • Configurations - Shipping Categories

The main admin menu has a new order of entries.

The main admin layout was de-defaced, in terms of testing I think a general test of menus, submenus and general page layouts should be enough.

Release notes

Changelog Category: Changed
Removed deface from a few parts of the backoffice thus making OFN less entangled with Spree code.

@luisramos0 luisramos0 self-assigned this Jul 16, 2019
In admin layout, only adds a div after the body tag.
…spree_frontend which is not used in ofn anymore
… in admin/order/show which is a view that is not used in ofn anymore, only admin/order/edit is used
@luisramos0 luisramos0 changed the title WIP Dedeface config WIP De-deface some admin configuration pages and the general admin layout and menu Jul 16, 2019
@luisramos0 luisramos0 changed the title WIP De-deface some admin configuration pages and the general admin layout and menu De-deface some admin configuration pages and the general admin layout and menu Jul 16, 2019
@luisramos0 luisramos0 removed the pr-wip label Jul 16, 2019
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.

I did not look at every line, to be honest. ;-)

Some could say that you added 250 lines of code to OFN that we now need to maintain. But if we maintain Spree code anyway, then you removed 130 lines from Spree+OFN code. 🎉
And the code is better structured and easier to maintain.

@mkllnk
Copy link
Member

mkllnk commented Jul 18, 2019

@luisramos0 One failing spec.

@luisramos0
Copy link
Contributor Author

luisramos0 commented Jul 18, 2019

and by that second line of thought, each spree module we remove from our list of dependencies represents thousands of lines of code we remove!

Copy link
Contributor

@sauloperez sauloperez left a comment

Choose a reason for hiding this comment

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

This is not easy to review. I trust you 👍

@RachL RachL self-assigned this Jul 22, 2019
@RachL
Copy link
Contributor

RachL commented Jul 23, 2019

  • Configurations - General Settings page

I change the configurations (cookies, title, terms and conditions...) all went well

  • Configurations - Image Settings
    A bit difficult to test, but adding / editing seems to work as usual.

  • Configurations - Shipping Categories
    Adding editing and deleting shipping categories is working good.

The main admin menu is working fine.
Menus, submenus and general page layouts are looking fine both on both latest Firefox and Chrome / Windows.

I tested super admin and manager's views. This is ready to go

@luisramos0 luisramos0 merged commit 1f75c7e into openfoodfoundation:master Jul 23, 2019
@luisramos0 luisramos0 deleted the undeface branch July 23, 2019 17:36
@luisramos0 luisramos0 restored the undeface branch July 23, 2019 17:39
@luisramos0 luisramos0 deleted the undeface branch July 23, 2019 17:50
@mkllnk
Copy link
Member

mkllnk commented Jul 24, 2019

I'm wondering why we didn't catch the breaking changes in here.

  • Did the deploy not work properly?
  • Or did we not test the pages that are broken?

@RachL Can you remember if you saw the affected parts when testing?

@RachL
Copy link
Contributor

RachL commented Jul 25, 2019

@mkllnk I had a quick look on the page layouts, but I didn't use ALL the pages except the ones listed above. So I may have miss these errors.
But I would like to be sure: what do you and @Matt-Yorkley think about when you say these are broken pages?
I'm noticing 2 things:
A. The table headers are bigger (which is ugly compared to the rest of the page but with my eyesight I'm actually for the first time reading them with no effort :) I'm looking forward to the day we change that and also that spree blue color for all text against a white background... but that's not the subject here :D )

B. On product, OC and subs pages, the icons are not centered

image

But these two things do not affect how the pages are working, or am I missing something?

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