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

Add some teal to represent successfully leased images #3854

Merged
merged 1 commit into from
Sep 15, 2022

Conversation

andrew-nowak
Copy link
Member

@andrew-nowak andrew-nowak commented Sep 7, 2022

What does this change?

Users have suggested that it would be useful to be able to see when an image has been leased more clearly. It is essentially visible on the image detail page (lease is large and visible, and the red danger banner goes yellow/warning once lease is added). But the flags on the search page and the status info boxes when multiple selecting could switch.

We've discussed colour and we are opposed to green (green should be reserved for free images only - paid/overquota/restricted images don't become free once leased) so we've picked teal #008080 (positive but not too positive).

We've also removed the blue stripe on leases - it showed if a lease was active but no one knew what it meant. The lease now fades out when inactive.

Some pics:

image
image
image

How should a reviewer test this change?

How can success be measured?

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)

@andrew-nowak andrew-nowak requested a review from a team as a code owner September 7, 2022 16:31
Copy link
Contributor

@twrichards twrichards left a comment

Choose a reason for hiding this comment

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

code looks great! I haven't tried it on TEST but I will add @paperboyo as a reviewer (from a usability perspective).

Also, just wanted bring to the attention of @ochiengolanga and @ornel-lloyd-edano as this will have an impact on BBC.

@louisegoodspeed
Copy link
Collaborator

louisegoodspeed commented Sep 8, 2022

Hiya, just had this forwarded to me by @ochiengolanga. Couple of thoughts -

Not sure the flag gives enough indication that it relates to a lease, users don’t automatically know what the symbols or colours mean on the flags. I wonder if our suggestion of adding a message if an image has a lease on selection would help with this need? This uses the colour #0050B3 and the copy could be something like e.g. - This image has a lease, please read all lease details before use.

Screenshot 2022-09-08 at 11 30 27

Could that work for your needs as well?

Also for if a lease has expired, greying it out or making it inactive, still shows the same messaging which could be confusing. Could we have something like the attached screen grab where it says that the lease has expired and doesn’t have "allow use" in the copy?

Screenshot 2022-08-09 at 10 26 31

Happy to discuss in Gridhour on Monday if that's better?

@paperboyo
Copy link
Contributor

paperboyo commented Sep 8, 2022

I like the idea of changing the copy on Inactive leases! Not sure how easy would it be to adjust it as in your example when the lease has not yet started (see above Starts in a day, Expires in 2 days). Maybe if that would be too hard, just sticking Inactive: in front would be enough? @andrew-nowak?

image

Regarding the messaging, I agree that users do not understand all the complicated colours and badges. But this PR makes it clearer as currently the leased state is not indicated at all in these three examples on a search page where this applies (actively allowed chargeable, over-quota and restricted). So we are not making anything worse, just universally better.

Also, this applies outside of selection state, so is more readily visible. And I don’t think it conflicts with your selection states work. I agree selection states overlays should indicate what this PR does in the more obvious way. We had reports of users asking for an already leased image to be leased and this should prevent that. A bit.

I think after the copy changes this should go in and we can see how it pans out in use and adjust if necessary. What do you think, @louisegoodspeed?

@louisegoodspeed
Copy link
Collaborator

louisegoodspeed commented Sep 8, 2022

@paperboyo yes the language is confusing when it comes to the lease not having started yet. In response to 'inactive' = would simply 'not started' work? Will have a ponder.
Can we change the green to red for an expired lease too?
Agree, both the flag and selection state warnings would work well together. In terms of colours we probably need to agree either the teal or blue... as I think we should use the same colour unless BBC can use our own?

@paperboyo
Copy link
Contributor

Can we change the green to red for an expired lease too?

Not really as two colours represent two states: allow and deny, so that would confuse things.

I think we should use the same colour unless BBC can use our own?

Whenever we could, we should try not to bloat configuration, so I think we should and can agree.

Teal suggests more allowing than blue, but not as much as green as Andrew mentioned. Blue is also used as UI accent colour, so is neutral when it comes to messaging.

We can trade mockups in Slack, will be faster than here, if you prefer.

@NickyPhelanBBC
Copy link
Collaborator

Hello, just chipping in re: colours.

Agree it would be good to reach an agreement if possible and the teal does look good with the red and yellow but unfortunately it's not going to work for us. Firstly we should really be using our enterprise colour palette (see attached) and secondly in the absence of a green elsewhere on the page the user is likely to think that the teal represents a green, as part of a traffic light system as there are already red and yellow elements on the page.

We'll have a think our end about other possible options (purple?) and perhaps we can discuss at Grid Hour next week.

Enterprise-colour-guide.pdf

@paperboyo
Copy link
Contributor

unfortunately it's not going to work for us. Firstly we should really be using our enterprise colour palette

So, maybe, we need to lift all colours into config? That seems… extreme? And in any case seems a bigger job than this PR. The doc does contain

image

I’m super-happy using the teal values from your doc! Deal? ;-)

the user is likely to think that the teal represents a green

Kinda does represent it (an active Allow lease makes an image usable). But only kinda, because it doesn’t make it free. It definitely represents (or – should) a move on the traffic light spectrum: a move towards “a green end”. Note that there is already this green here we are replacing (and the blue nobody understood):

image

Worth also noting, the icons retain their symbols (a pound, a graph and a flag), so the dual nature is not (completely) lost. Random colour from outside a traffic light spectrum would make less sense, I think. What we are trying to say is less: hey, there is a lease as hey, this is allowed.

Super-open to discussing on Slack/meeting, yeah!

@paperboyo
Copy link
Contributor

paperboyo commented Sep 13, 2022

Some conversation happened off-thread. This PR representing small change to leases was deemed OK to merge, but I wanted to record the points made too (pls feel free to add!):

  1. Leases should not use the same traffic lights colours (red, green) as they are there to modify Usage Rights. Some Usage Rights messaging is indeed modified by leases already at least for unpermissioned users (pending research into what we actually do). This PR changes the Allow leases to use the colour from the traffic light spectrum, but not green exactly
  2. Leases can be confusing, there may be multiple, contradictory ones added (not sure how to solve)
  3. Leases language may confuse users (inactive “Allow” on a unusable picture). This PR addresses this a bit by making inactive leases more obviously inactive (dimmed). Inactive-because-expired will look exactly the same as inactive-because-hasn’t-yet-started (not a new problem, though!)
  4. If both Allow and Deny active leases are on the picture, Allow wins. This is very bad and unsafe. Deny should win. (EDIT: When both Allow and Deny cropping leases are applied, Deny should win… #3860 opened)
  5. This PR addresses visibility of active Allow leases on a search page. It doesn’t do it for Deny leases (because Free to use pictures have no icon to easily colour). This is not immediate problem, but a limitation. (thumbnail underlines? but it will clash with [Org]-owned borders)
  6. It’s possible to add an Allow lease to a No rights image making it usable for unpermissioned users. It’s preferred that Usage Rights are set correctly instead. (disallow adding leases to No rights? luckily, Invalid images [no credit and/or description] are not made usable!)
  7. Everybody hates Grid date/time pickers, they are horrible and hard to use. But that’s nothing new.

@prout-bot
Copy link

Seen on auth, usage, collections, media-api, metadata-editor, thrall, leases (created by @andrew-nowak and merged by @paperboyo 18 minutes and 23 seconds ago) Please check your changes!

@prout-bot
Copy link

Seen on cropper, kahuna (created by @andrew-nowak and merged by @paperboyo 18 minutes and 27 seconds ago) Please check your changes!

@prout-bot
Copy link

Overdue on image-loader (created by @andrew-nowak and merged by @paperboyo 30 minutes and 4 seconds ago) What's gone wrong?

@prout-bot
Copy link

Seen on image-loader (created by @andrew-nowak and merged by @paperboyo 38 minutes and 36 seconds ago) Please check your changes!

andrew-nowak added a commit that referenced this pull request Oct 24, 2022
I did this in #3854 to try and override all other costs when an image is
missing credit or description (one definition of "valid"). Instead it
overrides all other costs whenever an image is uncroppable (restricted,
overquota, etc., the other definition of "valid").

costState is used to determine both which flags to show on the search
results page, and also the warnings/error messages on the image page,
which means that this change hid the restriction text from ALL
unpermissioned users! So revert this logic now, and commit to finding a
better way of applying the invalid flag in the future.
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.

6 participants