ministryofjustice / justice-gov-uk

Justice UK website
https://www.justice.gov.uk/
MIT License
2 stars 0 forks source link

CDPT-1544 Schedule page changes. #115

Closed EarthlingDavey closed 6 months ago

EarthlingDavey commented 6 months ago

@EmilyHazlehurst this is the mostly complete solution for having scheduled page revisions.

Todo:

EarthlingDavey commented 6 months ago

Hey @EmilyHazlehurst the changes look all good :D

I think because I opened the PR you need to click through to review and approve before merging.

wilson1000 commented 6 months ago

Hey @EmilyHazlehurst Some CSS suggestions from our conversation:

Front end to being the colour of the background in line with design:

.rvy_preview_msgspan {
  background-color: #373737 !important;
}

In the admin to remove the flash from the CSS hiding the PRO link

ul.wp-submenu-wrap {
  li:last-child {
    display: none !important;
  }
}
EarthlingDavey commented 6 months ago

@wilson1000 I think the last-child will be too wide sweeping. If theres a flash because the plugin is using js to cheekily change the url, it might be better to target both urls, something like:

a[href="admin.php?page=revisionary-pro"],
a[href="https://publishpress.com/links/revisions-menu"] {
  display: none !important;
}
wilson1000 commented 6 months ago

@wilson1000 I think the last-child will be too wide sweeping. If theres a flash because the plugin is using js to cheekily change the url, it might be better to target both urls, something like:

a[href="admin.php?page=revisionary-pro"],
a[href="https://publishpress.com/links/revisions-menu"] {
  display: none !important;
}

@EarthlingDavey I've tested locally; the selectors are too direct and cumbersome. For example; the DOM is under too much pressure using that selector. In addition, the CSS rule still overrides JS additions.

During the chat I had with Emily, the SCSS would look like this:

.toplevel_page_revisionary-q {
  ul.wp-submenu-wrap {
    li:last-child {
      display: none !important;
    }
  }
}

ul.wp-submenu-wrap only exists in the sidebar menu, is WordPress based and strong enough to withstand any future updates to URLs by the plugin people 👍

EarthlingDavey commented 6 months ago

It is always a bit risky using wide spanning selectors like that. For example you can see it does target some other menu items.

image

I wouldn't be too worried about dom pressure from 2 anchor selectors, I've seen worse.

Anyways, I'll hand the review over to you @wilson1000 because otherwise we might be going round in circles.

Give me a shout if you want me to dip back in.

wilson1000 commented 6 months ago

It is always a bit risky using wide spanning selectors like that. For example you can see it does target some other menu items.

image

I wouldn't be too worried about dom pressure from 2 anchor selectors, I've seen worse.

Anyways, I'll hand the review over to you @wilson1000 because otherwise we might be going round in circles.

Give me a shout if you want me to dip back in.

That's really helpful! Thanks so much for explaining mate.

wilson1000 commented 6 months ago

Hey @EmilyHazlehurst & @EarthlingDavey - will this be ok?

This will isolate the rule, also on the plugin page and remove the dependency on URLs

> ul.wp-submenu-wrap {
  li:last-child {
    display: none !important
  }
}
EarthlingDavey commented 6 months ago

@wilson1000 that looks good for admins. But the nuance of editors not having 'Settings' or 'Upgrade to Pro', means that for them 'Revision Archive' will be hidden.