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.

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

Conversation

Keith-CY
Copy link
Member

Keep the color of decimals consistent

Before:
image

After:
image

Keep the color of decimals consistent
Copy link

vercel bot commented Dec 21, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
ckb-explorer-frontend-in-magickbase-repo ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 21, 2023 8:49am

@Keith-CY Keith-CY changed the title fix: fix hover color of some ant components fix color of capacity change Dec 21, 2023
@@ -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

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

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants