Skip to content
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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

fix color of capacity change #180

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/components/TransactionItem/TransactionIncome/styled.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link

@WhiteMinds WhiteMinds Dec 21, 2023 •
edited
Loading

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:

  1. <DecimalCapacity lowlightDecimalPart={false} /> // lowlightDecimal default set to true
  2. <DecimalCapacity decimalPartClass={...} />

Copy link
Member Author

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

Copy link
Member Author

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

Copy link

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.

Copy link
Member Author

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

Copy link

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?

Copy link
Member Author

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?

Yes, this PR includes tiny updates and can be covered by #181 directly

color: inherit;
}

img {
width: 13px;
Expand Down