-
Notifications
You must be signed in to change notification settings - Fork 6
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
fix color of capacity change #180
Conversation
Keep the color of decimals consistent
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@@ -21,6 +21,9 @@ export const TransactionCapacityValuePanel = styled.div<{ increased: boolean }>` | |||
justify-content: flex-end; | |||
color: ${props => (props.increased ? props.theme.primary : '#FF6347')}; | |||
font-size: 16px; | |||
.monospace { |
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 approach relies on the implementation details of the child component.
Consider alternative approaches that involve exposing interfaces for control from the child component to the parent component:
<DecimalCapacity lowlightDecimalPart={false} /> // lowlightDecimal default set to true
<DecimalCapacity decimalPartClass={...} />
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 prefer not to add more options because it makes code hard to maintain. What if using *
selector to color all elements in the field
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 will refactor the component to accept style type
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 prefer not to add more options because it makes code hard to maintain. What if using
*
selector to color all elements in the field
I believe that implicit coupling is the key issue that increases the difficulty of maintainability. This incurs higher maintenance costs compared to adding a clearly defined and easily understandable interface.
The parent component should not rely on the implementation details of the child component. The more code like this exists in the project, the greater the cognitive burden when modifying certain shared components. It becomes more daunting to make changes because it's uncertain if it will cause issues with implicit dependencies on either side.
A more likely scenario is not being aware of the existence of implicit dependencies because there are no explicit dependencies to reference. This can lead to changes being made without realizing the impact on other parts of the code until later when issues arise.
Explicit dependencies, such as imports/exports, component interfaces exposed through props, or interfaces exposed through Context, allow us to immediately know which areas will be affected when modifying related code.
This can significantly reduce the probability of bugs, decrease the fear of modifying existing code for future maintainers, improve their maintenance speed, and enhance code readability.
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 Capacity
component is refactored by #181
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.
It seems that the current PR #180 is no longer needed?
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.
Keep the color of decimals consistent
Before:
After: