-
Notifications
You must be signed in to change notification settings - Fork 6
Overhaul icon presentation on Ada #1825
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
Draft
sjd210
wants to merge
27
commits into
main
Choose a base branch
from
improvement/ada-icon-refactor
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Instead using versions standardised to 24x24px and filling the full canvas, similar to Sci
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1825 +/- ##
========================================
Coverage 41.69% 41.70%
========================================
Files 542 542
Lines 23683 23690 +7
Branches 7875 7003 -872
========================================
+ Hits 9874 9879 +5
- Misses 13168 13765 +597
+ Partials 641 46 -595 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…efactor [VRT] Update baselines for improvement/ada-icon-refactor
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Standardises how most Ada icon SVGs are stored (other than ones for e.g. inequality and callouts), and refactors how these are presented to work in the same way as has been created for Isaac during the redesign (although not requiring any layering).
Firstly, for the SVGs themselves this has meant removing duplicate SVGs with the same icon but different colours. These are now black by default, and the colouring is handled on a per-icon basis in CSS instead (e.g. setting
"icon-color-secondary"for cyan in mostIconCards).These SVGs are also all square in dimensions with the icon taking up as much space as possible given that constraint (i.e. pressing against the left/right or top/bottom sides depending on the size of the icon itself). They previously had inconsistent spacing that made it hard to align icons in the same way except by adding margin in the css manually, which also limited the maximum size usable without adding negative margins. This change allows for icon size to be set and lead to a predictable outcome (e.g. setting "icon-md" for a 24x24px icon will lead to the same visual size of icon as any other put into the same position).
On the code side, this aims to reconcile some of the different ways we show icons to one standard form. There are still some exceptions where an icon is explicitly only used once and so can be linked directly (these exist for both Isaac and Ada), but generally most icons aim to be reusable.
For this reconciliation, this introduces an
IconPropsinterface used acrossHexIcon(and by extensionPageTitle),IconCard, andPromptBanner, as well as allowingstringinput for simple cases and backwards-compatibility. These props arrange into a structure like:specialised to each use-place such as using a different default size or colour, and applying the $SIZE to the hex background. Most of this isn't new, but
IconPropslets it be used in different (Ada-specific for now) places rather than write the whole structure manually as is done for one-off icon uses.In all this changes a lot of things very slightly (183 files changed 😱), including various icon sizes. I think these are for the better in terms of simplifying code and standardising presentation, but it's quite far-reaching so should be reviewed carefully. Any feedback appreciated.