-
Notifications
You must be signed in to change notification settings - Fork 114
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
🏷 Accessibility and Usability Improvements on Label Tab #1014
Changes from all commits
cc8d77b
5ef98cb
29d951d
1a0abae
5970f89
e24989f
7e04637
4341f4c
55cf019
3745535
e084b59
fd1ee53
b48196b
4cf9f58
4796626
aaf6c06
a72fd4f
473bf6c
6b6db8b
d81ba67
0b59436
81b882f
e1c13a3
efb9e50
12f81b2
686cf4e
65637a6
4e09b5b
011d9ba
cc991e6
8f6c778
da3d27c
1b5e519
aab2c13
83cd604
7f2bed8
76c4154
8f75fe0
6a654ae
364d1c4
33f7758
f1a905a
edd4ccf
616a439
1cf7e42
e4f3af4
423dbf6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
import { logDebug } from "./plugin/logger"; | ||
|
||
/** | ||
* @param url URL endpoint for the request | ||
* @returns Promise of the fetched response (as text) or cached text from local storage | ||
*/ | ||
export async function fetchUrlCached(url) { | ||
const stored = localStorage.getItem(url); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Future: I think you can theoretically reuse this to cache the nomimatim reverse-lookup as well although the key length may be too long and it is not that important going forward anyway. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The nominatim lookup is also cached using localstorage, but in a different way so that we can subscribe to its updates. Both implementations are temporary anyway because localstorage is not a good long-term solution and we need to find an alternative eventually (it depends on what we are allowed to use) |
||
if (stored) { | ||
logDebug(`fetchUrlCached: found cached data for url ${url}, returning`); | ||
return Promise.resolve(stored); | ||
} | ||
logDebug(`fetchUrlCached: found no cached data for url ${url}, fetching`); | ||
const response = await fetch(url); | ||
const text = await response.text(); | ||
localStorage.setItem(url, text); | ||
logDebug(`fetchUrlCached: fetched data for url ${url}, returning`); | ||
return text; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
/* React Native Paper provides an IconButton component, but it doesn't provide a plain Icon. | ||
We want a plain Icon that is 'presentational' - not seen as interactive to the user or screen readers, and | ||
it should not have any extra padding or margins around it. */ | ||
/* Using the underlying Icon from React Native Paper doesn't bundle correctly, so the easiest thing to do | ||
for now is wrap an IconButton and remove its interactivity and padding. */ | ||
|
||
import React from 'react'; | ||
import { StyleSheet } from 'react-native'; | ||
import { IconButton } from 'react-native-paper'; | ||
import { Props as IconButtonProps } from 'react-native-paper/lib/typescript/src/components/IconButton/IconButton' | ||
|
||
export const Icon = ({style, ...rest}: IconButtonProps) => { | ||
return ( | ||
<IconButton style={[s.icon, style]} {...rest} | ||
role='none' focusable={false} accessibilityHidden={true} /> | ||
); | ||
} | ||
|
||
const s = StyleSheet.create({ | ||
icon: { | ||
width: 'unset', | ||
height: 'unset', | ||
padding: 0, | ||
margin: 0, | ||
}, | ||
}); |
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.
This is fine for this release, which I really don't want to make other changes to, but let's test out whether turning off pagination deals with autoscrolling properly and then turn off pagination by default. I think that is what we decided.
I think we will still need this - what if there is only one "yes/no" question - but we might be able to remove it then.
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.
Pagination being on or off isn't something we control from the phone code; it's actually an option in the forms and can be enabled/disabled from KoboToolbox or the Excel file or the XML file. When I brought up the topic, I was just suggesting that we change the built-in demographic survey to not use pagination.
But if someone else wants to supply their own surveys that do have pagination turned on, they can do that as much as they please.
The mock Time-Use survey we have been using does not use pagination and the scrolling has been fine.
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.
Right, I was suggesting that we turn pagination off for the default survey.
I know that the mock time use is not paginated, but it is also not very long.
I want to try it with a long survey, like the demographics, and see if the experience is smooth - e.g. does it automatically scroll down when a question is complete, and whether that feels weird or not?