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

chore(Design): Modal Cancel Button #2344

Merged
merged 4 commits into from
Jun 5, 2023
Merged

Conversation

poolsar42
Copy link
Collaborator

Description

Positions close modal button inside the modal.

Related Issues

Testing

Visual changes were tested on all components locally

Checklist

  • I have read the CONTRIBUTING guidelines
  • I have tested my code (manually and/or automated if applicable)
  • I have updated the documentation (if necessary)

@poolsar42 poolsar42 self-assigned this Jun 1, 2023
@poolsar42 poolsar42 force-pushed the chore/2308-modal-cancel-button branch from f8b38fa to 64410cb Compare June 1, 2023 15:47
@poolsar42 poolsar42 marked this pull request as ready for review June 1, 2023 16:55
@poolsar42 poolsar42 force-pushed the chore/2308-modal-cancel-button branch from f0f3e9d to 6183ea6 Compare June 2, 2023 05:26
e.rc.addr_type === CryptoAddressType.ETH
})
.map((address) => {
return address.qc.alias.toLowerCase() as string
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alias for SC wallet is not an Ethereum address, but a Nickname
Because of it, it throws errors from nft gallery that

owner should be a valid address or ENS name

Copy link
Contributor

@Cosmin-Parvulescu Cosmin-Parvulescu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks OK, some things to consider:
1 - Can these modals be further consolidated into two different types, they seem to share a lot of properties and maybe could be extracted as components (mostly the outer divs)
2 - In Tailwind CSS v3.0, transform and filter utilities like scale-50 and brightness-75 will automatically take effect without needing to add the transform, filter, or backdrop-filter which means we can remove the transform property

@poolsar42
Copy link
Collaborator Author

Looks OK, some things to consider: 1 - Can these modals be further consolidated into two different types, they seem to share a lot of properties and maybe could be extracted as components (mostly the outer divs) 2 - In Tailwind CSS v3.0, transform and filter utilities like scale-50 and brightness-75 will automatically take effect without needing to add the transform, filter, or backdrop-filter which means we can remove the transform property

No more transform properties, and I also think consolidation should happen at some point.
Do you think this PR is this point?

@poolsar42 poolsar42 force-pushed the chore/2308-modal-cancel-button branch from 164a6b9 to a4a8238 Compare June 3, 2023 02:49
@betimshahini betimshahini merged commit a1e7cd6 into main Jun 5, 2023
16 of 20 checks passed
@betimshahini betimshahini deleted the chore/2308-modal-cancel-button branch June 5, 2023 13:28
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.

chore(Design): Modal Cancel Button
3 participants