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

Put logic in place for transactions view #2260

Merged
merged 2 commits into from Apr 19, 2019
Merged

Conversation

bkase
Copy link
Member

@bkase bkase commented Apr 18, 2019

Logic in place for rendering all transactions in the spec.

Some layout is completed, but styling still needs a ton of work.

Tables were used as this is semantically a table, but it's unclear
whether it should just all be flex or not.

TransactionsView for now just hardcodes all the cells present in
the mockup to make sure we can look at all of them (for the
purposes of testing).

The Spec (non-expanded)

Screen Shot 2019-04-17 at 5 12 43 PM

The Implementation (non-expanded)

Screen Shot 2019-04-17 at 5 04 53 PM

The Spec (expanded)

Screen Shot 2019-04-17 at 5 12 48 PM

The Implementation (expanded)

Screen Shot 2019-04-17 at 5 05 40 PM

@bkase
Copy link
Member Author

bkase commented Apr 18, 2019

We can cleanup the minus signs in a later PR

Copy link
Member

@nholland94 nholland94 left a comment

Choose a reason for hiding this comment

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

Approving as we will land this as is for now, but when we move forward to fully style this component, I think we will need to switch to CSS Grid. Flexbox is no good here due to the complex alignment across multiple sections of elements in the dom, and tables are basically the C++ of CSS (footguns galore).

Logic in place for rendering all transactions in the spec.

Some layout is completed, but styling still needs a ton of work.

Tables were used as this is semantically a table, but it's unclear
whether it should just all be flex or not.
@bkase bkase added the ready-to-merge Adding this label will trigger mergify and trigger CI to run and merge the PR label Apr 19, 2019
@mergify mergify bot merged commit de4194d into master Apr 19, 2019
@mergify mergify bot deleted the transactions-view-logic branch April 19, 2019 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge Adding this label will trigger mergify and trigger CI to run and merge the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants