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

Update palette colours #51

Merged
merged 11 commits into from
Jun 11, 2024
Merged

Update palette colours #51

merged 11 commits into from
Jun 11, 2024

Conversation

ab-gnm
Copy link
Member

@ab-gnm ab-gnm commented Jun 6, 2024

Description

Updates palette colours based on @akemitakagi 's feedback document.

I've left the opinion_300 in but marked it as deprecated with recommendation to use 400 instead.

Also added a sheet with all palette pills to the sample app (screenshot below).

Testing notes/instructions:

Build and run the sample app to see all the palette colours, and maybe confirm them with Akemi's sheet.

Checklist

  • Changes have been checked by the developer
  • Changes have been checked by the reviewers
  • Unit tested
Recommended reviewiers
  • Design review for UI changes from @guardian/design-system
  • Code review from @guardian/android-developers or @guardian/ios-developers
  • Optional code/API review from @guardian/client-side-infra

For pull requests introducing UI changes:

  • Sign-off by Design: @akemitakagi
  • Dark Mode
  • Tablet
  • Accessibility (e.g. VoiceOver):
Screenshots or videos:

Palette

@ab-gnm ab-gnm requested review from akemitakagi and a team June 6, 2024 15:32
Copy link
Member Author

Choose a reason for hiding this comment

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

Changes in this file are to create a bottom sheet scaffold. The bottom sheet is used to display the palette when "Open Palette" button is clicked.

Copy link
Member Author

Choose a reason for hiding this comment

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

Colour changes are in this file.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added this xml file from news app and updated colours in it too. This will replace the file in source module in the news app.

@ab-gnm ab-gnm marked this pull request as ready for review June 6, 2024 15:33
@ab-gnm ab-gnm added the Android Android specific PRs label Jun 6, 2024
@ab-gnm ab-gnm changed the title Ab/update colour palettes Update palette colours Jun 6, 2024
@akemitakagi
Copy link

akemitakagi commented Jun 7, 2024

Hi Adi,

All the colours look good apart from the opinion of 300 and missing Special Report and Special Report Alt colours.

  • opinion.300 is deprecated.
    If there are existing components using opinion.300, we suggest to use opinion.400.

  • I added the Special Report and Special Report Alt colours in the doc I shared.
    Please let me know if there is any questions.

https://docs.google.com/document/d/1chQ2EqUdkjvzqn8hHgAthCL84M-XWeQ2hQg0_BsrBsg/edit

@jamesmockett
Copy link

On web we have specialReport.450 and specialReport.700 which aren't included here. We also have a set of specialReportAlt colours. Are these deliberate omissions as they're not used in apps? (This is probably a question for @akemitakagi 😄)

@akemitakagi
Copy link

Hi @jamesmockett,
Yes, we have Special report and Special report Alt colours.
I was waiting for the confirmation of the final colours as there was confusion about the signed-off colours.

The Special report colours are correct in Source, so I will let Adi know about the additional colours.

@akemitakagi
Copy link

Hi @ab-gnm,
All the colours look good. Thank you!

Copy link
Contributor

@EnochHankombo92 EnochHankombo92 left a comment

Choose a reason for hiding this comment

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

LGTM

@ab-gnm ab-gnm added this pull request to the merge queue Jun 11, 2024
Merged via the queue into main with commit b2f1d62 Jun 11, 2024
6 checks passed
@ab-gnm ab-gnm deleted the ab/update-colour-palettes branch June 11, 2024 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Android Android specific PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants