-
Notifications
You must be signed in to change notification settings - Fork 91
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
Color-Style System Re-Structuring and Colorblind Accessibility #1384
Color-Style System Re-Structuring and Colorblind Accessibility #1384
Conversation
Hi @chinweibegbu - can you please capture a short video showcasing the new functionality? Thank you for submitting! |
Also @chinweibegbu do not worry about the failing deployment. This is not due to anything you did- just the way our deployments are set up. |
0ea797f
to
8bf199d
Compare
Add default and example color scheme objects to colors.json. Minify colors.json in build_data.js. Add colors.json to DataLoaderSystem's fileMap. Add color scheme helper functions to StyleSytem, with comments to describe each functions parameters and return values (where applicable). Update StyleSystem's styleMatch() function to interact with color scheme objects from colors.json rather than from STYLE_DECLARATIONS. Fix white space discrepancies in StyleSystem. Update color_selection.js and colorblind_mode_options.js to use StyleSystem rather than the non-existent ColorSystem. Uncomment color schemes and colorblind mode options sections in preferences.js to add them to the Preferences pane of the UI. Refs: Issue facebook#1230
Extract the style definitions to a separate folder, including style definitions which were in colors.json. Add abstracted color and style definitions to minification and data loading files. Load the style definitions alongside color loading in StyleSystem.js. Update style selectors to use newly abstracted style definitions. Map colors defined in the color system to their hex codes. in StyleSystem.js. Reduce the boilerplate code in style matching in StyleSystem.js.
Add default color scheme property to StyleSystem.js to hold the default color scheme loaded from color_schemes.json. Abstract out HEX color code determination in styleMatch() into its own function, getHexColorCode. Define getHexColorCode() such that it fetches the HEX color code from the default color scheme if it is not defined in the current color scheme to increase efficiency and reduce the amount of information that has to be defined in a new color scheme object. Optimize color scheme definitions in color_schemes.json to utilize the performance upgrade in getHexColorCode() by removing key-value pairs already in the default color scheme. Add another example colorblind-friendly color scheme. Correct spelling mistake in helper function description.
bb79e9c
to
d693ce2
Compare
NOTE: Also includes rebasing merge conflict handling
d693ce2
to
dd711f7
Compare
Cool, this looks very well done, I don't have many comments yet, but will watch as it develops. I have some concerns about adding a keyboard shortcut for this feature, only because we've regularly seen users get confused about the "wireframe" feature - it's common for users to activate that mode accidentally and not know how they got there or how to get back to normal rendering. (This is a separate issue that we should address which has nothing to do with this PR). But I'm just more wary now about adding keyboard shortcuts that change the rendering. |
👏 @chinweibegbu this branch looks fantastic. There are some small test errors with
Note: There will be a lot of warnings that you didn't cause- don't sweat warnings about 'unexpected TODO', those aren't your problem. Just try and fix any warnings/errors that you see in code you touched. For example, here are the all errors I see currently (plus a few warnings you should also try to clean up):
|
I wonder, what are your thought on the URL state for this feature. I think this UI state should be shareable via URL. And the since there might be other color styles in the future, those probably as well. |
I was originally thinking that this feature could be a personal preference, not a URL param (since color preference would be highly personal, esp. if a color correction is applied for colorblindness). |
My use first use case is that organizations like https://sozialhelden.de/ who maintain https://wheelmap.org/ would want to link to this editor with the style active from an email, blogpost, tutorial. The UX would be a lot better with a deeplink than having to explain "follow the link, then find the right icon on the right, then find the section x, then select the radiobutton Y". Even I have trouble finding anything in the right sidebars ;-). The other thing is that once #1394 or similar lands, this becomes even more a application state that should be shareable with others. Like from a Tasking Manager kind of app that links to Rapid with the pedestrian focus on. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not certain of the name and location of this file. It being called README.md
in the /data/
folder implies that it describes the content of everything in that folder, which is not the case.
The file details how Rapid deals with colors and styles relative to structure. If there were a specific folder that held all information related to colors, I would have put it there. Alternatively, it could be renamed to COLORS.md
or COLOR-STYLE.md
.
Another thing to consider is whether documentation of this nature even meant to be in this repo in the first place. For instance, I have been informed of Rapid's Wiki pages by @Bonkles. I think it is important to keep track of the way developers are thinking about Rapid's structure but, in summary, I do not know that I am doing this in a manner that matches the current structure of Rapid.
Comments on the file content itself would be appreciated as well.
Focus: Inadequate current state [Documentation]
CONTEXT: There are certain colors which are very close in HEX value e.g.
Focus: Food for thought [Structure] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current color system attempts to group and order colors in an intuitive manner. Colors are first grouped into their nearest common color name e.g. red
, blue
. Then, they are ordered in terms of relative brightness/lightness.
Current issues/thoughts:
-
Adding a new color which does not fall at either extreme of a particular color group's spectrum based on brightness/lightness affects every other color in its color group (see Fig. 2)
-
Updating the order of colors requires refactoring which is error-prone (see Fig. 2)
- Potential solutions
- Use a placeholder key to move from the first number-wise to the last
- Ensure to refactor code outside
color_schemes.json
in the reverse number order --> If you update allblue-4
references toblue-5
first, allblue-3
references can be confidently updated toblue-4
NOTE: These solutions are still error-prone as they require a certain level of attention to detail and care.
- Potential solutions
-
Does the current system really order the colors within a color group intuitively? Is there a better and more deterministic way to order the colors within a color group (e.g. based on the HEX value)? I believe a UI/color expert would need to look at this system to determine the answer to this question.
-
How does a colorblind developer think about this color system? (one would think they wouldn't be involved with color definition, being colorblind and all but you never know)
---
title: "Fig. 2: Add New Color + Update Order (Illustration)"
---
flowchart TD
A([AIM: Add a new color between `blue-2` and `blue-3`]);
B[Rename `blue-3` to `blue-4`];
C[Rename `blue-4` to `blue-5`];
D[/Add new value-pair for updated `pink-3`/]
E[Update all existing references to `blue-3` to `blue-4`]
F{How do I add update all the references\n that were originally `blue-4` to `blue-5` without\n including those added in the last step?};
A --> B --> C --> D --> E --> F;
Focus: Food for thought [Structure]
Updates:
|
Add `showColorblindSimulators` to the array of poweruser features. Add strings for the `showColorblindSimulators` feature in the poweruser pop-up to `core.yaml`. Add `_initHide` property to `section.js` to enable initializing any section `<div>` with a `.hide` class in a non-permanent manner. Call the `initHide()` function with D3.js chaining to set the `_initHide` property of the to `true`, which means that `uiSectionColorblindModeOptions` section should initially be hidden. Add custom on-toggle behavior for `showColorblindSimulators` feature such that the state of the poweruser feature checkbox associated with it adds or removes the `.hide` class from the section's `<div>`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to make the colorblind_mode_options.js
section poweruser
-only, I create a new property _initHide
to allow initializing a section <div>
with the class .hide
, as defined in 80_app.css
. This .hide
class then added or removed in the toggleFeatures()
function of the rapid_poweruser_features_dialog.js
file.
My concern is that I needed to essentially duplicate the _shouldDisplay
property in rapid_poweruser_features_dialog.js
because this property completely deletes the section rather than just hiding it, which is not what I want to do.
- What is the expected behaviour of
_shouldDisplay
? - Should it be completely removing the section from the DOM (a.k.a. setting its
html
to''
)? - What are the situations in which a section is coded and then we would want it to be permanently deleted?
Based on its usage (see the image above), I think a more appropriate name would be _neverDisplay, to communicate that you can never get it to display again. However, I wonder if one might want to render the section being essentially deleted again in some of these use cases. If that is the case, I think _shouldDisplay
should function the way _initHide
functions right now, which is easily accomplished by removing lines 106 - 109. Else, _initHide
covers this new potential possibility.
As the author of this functionality, I would love your input, @bhousel.
Focus: Food for thought [Efficiency]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you do something like this to check if the user has the poweruser flag set:
sectionShouldDisplay() {
const urlhash = context.systems.urlhash;
return urlhash.getParam('poweruser') === 'true';
}
This seems to be how code in other parts of Rapid checks for the flag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue with the usage of sectionShouldDisplay()
is that its return value gets passed to the .shouldDisplay()
function of the section instance (see image below) and that property completely deletes the section. This behavior contradicts the logic for how I toggle the section on and off and it essentially deleted the <div>
section I would be adding or removing the .hide
class from.
While I can define a function to check that the parameter is set, it wouldn't change the property I'm using - _initHide
instead of _shouldDisplay
. Am I understanding you correctly?
There's lots of interesting work done here, but I'm not sure what to do with it, so I'll close for now. I do appreciate it, and it will inform some of our decisions about styling that we still need to figure out.. On colorblindness: There are a bunch of browser extensions for simulating different kinds of color blindness, so I don't think this is something that we need to build into Rapid. I think the hope is that we can use Pixi's color system to correct for different kinds of color blindness, which is a bit different from this. On style refactor: Moving the style definition out of StyleSystem and into its own file is a good change, (and we also want to eventually allow users to supply their own style document, somehow) but we still don't have anything close to a specification of a "style document for Rapid" will have in it (#451), so it seems a bit premature. |
TL;DR
poweruser
-accessibleColor & Style System Re-Structuring
Color System Re-Structuring
colors.json
andStyleSystem.js
into a new file,/data/color_schemes.json
/data/color_schemes.json
files inbuild_data.js
and add it toDataLoaderSystem
'sfileMap
variable/data/colors.json
fileStyle System Re-Factoring
STYLE_DECLARATIONS
object into to a new, separate file,/data/styles.json
, to abstract style definition away from style management/manipulation./data/color_schemes.json
files inbuild_data.js
and add it toDataLoaderSystem
'sfileMap
variableStyleSystem.js
to use the newly-created color system and the abstracted style definitions usingDataLoaderSystem
and a new helper function,getHexColorCode()
StyleSystem
'sstyleMatch()
function to set style properties in a more efficient manner with JavaScript shorthandColorblind Accessibility
styleMatch()
function to interact with color scheme objects from colors.json rather than fromSTYLE_DECLARATIONS
StyleSystem.js
high_contrast
to reflect functionalityStyleSytem.js
color_selection.js
andcolorblind_mode_options.js
to useStyleSystem
rather than the non-existentColorSystem
classpreferences.js
to add them to the Preferences panecolor_schemes.json
to illustrate the process of adding a new color scheme as well as attempt solving Rapid's colorblind inaccessibility problemColor.Schemes.and.Colorblind.Simulators.-.DEMO.mp4
ALT
+SHIFT
+C
) to iterate through available color schemes without use of the Preferences panepoweruser
-only featureRefs: Issue #1230, PR #1365