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

T1629 - interim filters #4263

Merged
merged 15 commits into from
Jun 3, 2024
Merged

Conversation

AndyKilmory
Copy link
Collaborator

@AndyKilmory AndyKilmory commented Apr 11, 2024

What does this change?

This change introduces a 2nd top-bar at the top of the Grid UI to accommodate increased complexity in the filtering, sorting and permissions controls. It introduces a new permissions filters which acts to provide an easy way to define combinations of chips and the chargeable setting when searching images.

Screenshot 2024-04-30 at 12 13 57

Screen shot show the new layout including the new Interim Filters control (written in React), the new Sort Control moved to the 2nd tier (already merged in pervious PR) and a new My Uploads control to improve flexibility over styling.

The contents of the Interim Filter drop down are controlled through configuration;

interimFilterOptions = [
{
id:"allPermissions",
label:"All permissions",
mapping:"",
payable:"none"
},
{
id:"usableForAll",
label:"Usable for all",
mapping:"is:BBC-owned,-has:restrictions",
payable:"false"
},
{
id:"usableForNews",
label:"Agency (Usable for News)",
mapping:"category:agency",
payable:"false"
},
{
id:"bbcOwned",
label:"BBC owned programmes",
mapping:"category:programmes-organisation-owned",
payable:"none"
},
{
id:"independent",
label:"Independent programmes",
mapping:"category:programmes-independents",
payable:"none"
}
]

The use of Interim Filters is controlled use 'usePermissionsFilter' config variable in kahuna (true or false).

How should a reviewer test this change?

Ensure all controls work as described, work for an accessibility perspective and adapt correctly to screen width changes

Who should look at this?

Tested? Documented?

  • locally by committer
  • locally by Guardian reviewer
  • on the Guardian's TEST environment
  • relevant documentation added or amended (if needed)

@paperboyo paperboyo added the bbc label Apr 25, 2024
@AndyKilmory AndyKilmory force-pushed the t1629-interim-filters-separated branch from fc156dc to 2d3e230 Compare April 30, 2024 11:04
@AndyKilmory AndyKilmory marked this pull request as ready for review April 30, 2024 11:13
@AndyKilmory AndyKilmory requested a review from a team as a code owner April 30, 2024 11:13
@paperboyo paperboyo mentioned this pull request May 3, 2024
4 tasks
@AndyKilmory AndyKilmory force-pushed the t1629-interim-filters-separated branch from 0a50e3d to 2fb22bd Compare May 8, 2024 09:07
@AndyKilmory AndyKilmory force-pushed the t1629-interim-filters-separated branch from f846116 to 7658b8d Compare May 13, 2024 15:19
@paperboyo
Copy link
Contributor

paperboyo commented May 14, 2024

Thank you! For now, minor points regarding config and naming:

  1. Could we (can defo be later!) document all the extensive config here and in PR description (doesn’t mention kahuna.conf, permissionsDefault, what payable does and what options it offers, etc)?
  2. I think we should go back and rename interimFilterOptions to permissionsOptions (or even permissionsFilterOptions?). I think the fact this is “interim” is not that helpful and having everything refer to permissions or permissionsFilter would make it clear this is all related? (so maybe permissionsFilterDefault too)?

@paperboyo
Copy link
Contributor

TL;DR: we will give it a code review and can merge if you are happy (only pt. 6 may need fixing as otherwise we have a hard exclusionary behaviour between disparate config options; @AndyKilmory is aware). The below is for your consideration…

I have tested it a bit more. In general, I think this is a distinctly BBC-only functionality for several reasons, so it may be that the following problems are squarely in your court: we don’t wanna be a blocker with something we are not likely to use. On the other hand, this may be the first piece of functionality which cannot simply be configged-in: some of the following would need to be fixed before that would be possible (ie. before we, or any non-BBC user, could switch this on)…

Some of the reasons we can’t use it, forgetting the fundamental one: we can’t think of useful “permissions” filter for the Guardian as-is, so no real need from us to fix it, let alone quickly or before merge (but still, hopefully, useful):

  1. This, when ON, hard-codes BBC-specific Show payable images. At the Guardian, and as is default by-config, we use Free to use instead. It differs in some fundamental and some trivial ways too:
    a) ours/default is checked, BBC – unchecked
    b) ours/default responds to show_paid permission (it controls initial state of the checkbox). This doesn’t work now in this PR: the default state doesn’t correctly respond to the permission.
    c) [trivial] this uses BBC-specific toggle UI (even though there are still checkbox UIs and it collapses into a checkbox UI itself)
  2. [trivial, but consistency matters] this uses new type of checkbox UI (for eg. My uploads): they have no hover state unlike default Grid ones (bad), but they have unselectable text in labels (good!)
  3. this introduces a third (sic!) dropdown UI. We already have two (one too many), not sure why we need a third one:
    image
  4. [as mentioned off-github] this doesn’t work with filters.shouldDisplayOrgOwnedCountAndFilterCheckbox = true:
    a) the checkbox ([org]-owned) isn’t displayed at all
    b) the app goes into a spin and takes down browser in this scenario:
    permissions bug

Here are some general thoughts unrelated to the above. Consider only if useful for you:

  1. Your usableForAll config (is:BBC-owned,-has:restrictions) would be more useful as is:BBC-owned,-has:restrictions OR is:BBC-owned,has:restrictions,leases.leases.access:allow-use,leases.leases.active:true, to take into account Restricted imagery which has been granted an Allow cropping lease which is active, but this isn’t possible yet:
    a) we don’t have OR, which, at first, we could allow in search aliases config (to not have to deal with user-facing UI complexity upfront)
    b) leases may need re-modelling (? to be confirmed by an engineer): it is not possible to search for an effectively active lease of certain type (ie. I think ?query=leases.leases.active:true leases.leases.access:allow-use search will return an image with and active Deny cropping lease and not-active Allow cropping one (not intended). This limitation prevents us from making this permission more useful and from using leases in searching.
    c) also see When both Allow and Deny cropping leases are applied, Deny should win… #3860
  2. When default Grid collapses UI, Search filters dropdown gathers more and more checkboxes:
    image
    With this ON, Search filters only ever contains the Date filter (which shows up too high too). It should behave like in default Grid (because of space, but also label makes no sense otherwise):
    image
  3. Permissions filter label (here customised to say All images) first collapses to generic Permissions and then to just a chevron:
    image
    a) I think it should always use the configured label
    b) it should collapse under Search filters (like default Free to use does), so that it doesn’t have to do 9a and it wouldn’t ever look like a stray chevron

@AndyKilmory
Copy link
Collaborator Author

Thanks @paperboyo - will look to fix blocking issues asap and raise the others with the team for discussion.

@AndyKilmory
Copy link
Collaborator Author

Hi @paperboyo, Have pushed changes that I believe fix all the functional issues raised regard the PR. Please could it be re-reviewed? (I'm on leave next week 20th to 24th May so no great rush). Thanks

Copy link
Contributor

@dblatcher dblatcher left a comment

Choose a reason for hiding this comment

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

looks good - I'd just suggest adding a comment near the "permissionsDefault" config to avoid misunderstanding about what it does.

@dblatcher dblatcher merged commit aabe03e into guardian:main Jun 3, 2024
1 check passed
@prout-bot
Copy link

Seen on auth, usage, image-loader, leases, cropper, kahuna (created by @AndyKilmory and merged by @dblatcher 9 minutes and 38 seconds ago) Please check your changes!

@prout-bot
Copy link

Seen on metadata-editor, collections, media-api (created by @AndyKilmory and merged by @dblatcher 9 minutes and 43 seconds ago) Please check your changes!

@prout-bot
Copy link

Seen on thrall (created by @AndyKilmory and merged by @dblatcher 9 minutes and 46 seconds ago) Please check your changes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants