-
Notifications
You must be signed in to change notification settings - Fork 13
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
More UI customization #41
base: main
Are you sure you want to change the base?
Conversation
Hey, sorry! This was just a local error — my apologies!
On Apr 23, 2024, at 5:00 PM, Robin ***@***.***> wrote:
@robonyong commented on this pull request.
________________________________
In src/Character/index.tsx<#41 (comment)>:
@@ -2,14 +2,14 @@ import React, { useEffect, useState } from 'react';
import Panel, { FlipPanel } from '../Panel';
import defaultSound from '../assets/flip.mp3';
-// @ts-expect-error the minified file is not in the type declarations
-import { Howl } from 'howler/dist/howler.min.js';
Is this just a local conflict or are you also proposing to remove the sound playback feature?
—
Reply to this email directly, view it on GitHub<#41 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AXDU7BMVXHM22NNKG6OPRE3Y63DVPAVCNFSM6AAAAABGU7YXGOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAMJYGI4TCNZWGI>.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
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.
Overall looks good, I have some concerns/preferences that I'd like addressed before pulling this in
|
||
background: var(--background); | ||
display: flex; | ||
justify-content: center; | ||
width: var(--character-width); | ||
padding: 0.5em; | ||
position: relative; | ||
border-radius: var(--border-radius); | ||
margin-right: var(--margin); |
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.
As it is I think this overlaps in function a bit with border-width
. I understand why you'd want to break the character spacing away from border-width, so two requests:
- consider renaming this prop to something more descriptive like
characterSpacing
, css variable name--character-spacing
- Implement this on the wrapper level instead (https://github.com/robonyong/react-split-flap-display/blob/master/src/SplitFlapDisplay/styles.module.scss#L13) and replace the border-left with margin-left and your css variable
--font-size: 1rem; | ||
--padding: 10px; |
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 think this type of overall padding is best left to the parent container of this component, and not imposed by the library. please 🔪
|
||
display: flex; | ||
color: var(--color); | ||
font-size: var(--font-size); | ||
font-family: var(--font-family); // Apply font family | ||
padding: var(--padding); |
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.
🔪
Updates: