Animation: Direction UI Component #4143
Conversation
littlemilkstudio
requested review from
barklund,
BrittanyIRL,
carloskelly13 and
mariano-formidable
|
Size Change: +5.28 kB (0%) Total Size: 1.16 MB
鈩癸笍 View Unchanged
|
| <Label key={direction} htmlFor={direction} direction={direction}> | ||
| <HiddenCaption> | ||
| {sprintf( | ||
| /* translators: %s: story title. */ |
There was a problem hiding this comment.
nit: think ya mean which direction not story title?
There was a problem hiding this comment.
lol. yeaup
| <RadioGroup> | ||
| {directions.map((direction) => ( | ||
| <Label key={direction} htmlFor={direction} direction={direction}> | ||
| <HiddenCaption> |
There was a problem hiding this comment.
Since this span is hidden and only serving to indicate to screen readers what the labeling is, you're better off using aria-label on the Label and removing HiddenCaption.
There was a problem hiding this comment.
oooo this is the magic I was looking for. thanks!
| {sprintf( | ||
| /* translators: %s: story title. */ | ||
| __('%s Direction', 'web-stories'), | ||
| direction.replace(/([a-z])([A-Z])/g, '$1 $2') |
There was a problem hiding this comment.
nit: this is resulting in: bottom To Top Direction casing. Feel like the first word should also be capitalized?
There was a problem hiding this comment.
Good call. will update
| input:focus ~ ${DirectionIndicator} { | ||
| outline: 5px auto -webkit-focus-ring-color; | ||
| } |
There was a problem hiding this comment.
This isn't showing an outline on firefox?
There was a problem hiding this comment.
Thanks for pointing out, will trouble shoot
Ahh good catch. updated the prop from |
Codecov Report
@@ Coverage Diff @@
## main #4143 +/- ##
===========================================
+ Coverage 69.63% 82.46% +12.83%
===========================================
Files 792 812 +20
Lines 13913 14146 +233
===========================================
+ Hits 9688 11666 +1978
+ Misses 4225 2480 -1745
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| <Label | ||
| key={direction} | ||
| aria-label={sprintf( | ||
| /* translators: %s: which direction. */ |
There was a problem hiding this comment.
Maybe give an example? e.g.
| /* translators: %s: which direction. */ | |
| /* translators: %s: Direction, for example 'top' or 'left'. */ |
There was a problem hiding this comment.
Actually, it looks like direction will bee something like rightToLeft, and then you are transforming this to right-to-left? But right-to-left cannot really be translated? 馃
I would change this to:
__( 'Direction: %s', 'web-stories' ) and then you'll need to make all the directions translatable, e.g. __('right-to-left'), etc. And no, __(direction) doesn't work :-)
Hope that makes sense.
| const camelToPascal = (string) => | ||
| string.charAt(0).toUpperCase() + string.slice(1); | ||
|
|
||
| const pascalToSentance = (string) => string.replace(/([a-z])([A-Z])/g, '$1 $2'); |
|
@littlemilkstudio My comment doesn鈥榯 seem to have been fully addressed? Can you please open a follow-up ticket to make the directions fully translatable? |
swissspidy
added
Group: Animations
Type: Enhancement
Summary
Adds a custom radio input for selection of our directions.
Relevant Technical Choices
Felt like a radio input, so I made it one 馃
To-do
NA
User-facing changes
none
Testing Instructions
run storybook ->
Animations / Direction Radio Input-> click & up/down arrow through to options to validate keyboard accessibility.Also added unit tests which will be validated by CI.
Fixes #3794