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

Viewing room list cards #3457

Merged
merged 30 commits into from
Jul 13, 2020
Merged

Viewing room list cards #3457

merged 30 commits into from
Jul 13, 2020

Conversation

pvinis
Copy link
Contributor

@pvinis pvinis commented Jun 15, 2020

Keep in mind:

  • Using cards from palette.

  • Removed an unused file still left from storybook.

  • Added RelayEnvironmentProvider to be able to use relay-hooks.

  • Now with pagination.

  • iPad layout supported.

  • Pair with @mdole on making some tests.

  • Update to palette after the pending one.

Screenshot 2020-07-07 at 15 14 54

Screenshot 2020-07-07 at 15 31 34

Screenshot 2020-07-07 at 15 31 49

@pvinis pvinis marked this pull request as ready for review June 17, 2020 15:42
@pvinis pvinis requested a review from mdole June 17, 2020 15:42
@mdole
Copy link
Contributor

mdole commented Jun 18, 2020

huh @pvinis I checked out the branch and tried opening a VR from the sim - still took me to the screen within the app and not the web view https://share.getcloudapp.com/YEupoEdE

no clue what's up there. maybe ask the MX team tomorrow?

@pvinis
Copy link
Contributor Author

pvinis commented Jun 18, 2020

It was that I didn't have the lab option enabled on my sim 🤦. Interesting how everything else worked so far with that being off. Anyway, this should be ready to be merged now 👍.

Copy link
Contributor

@mdole mdole left a comment

Choose a reason for hiding this comment

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

Awesome! Great job @pvinis. Only questions I have are:

  1. Should we be using Styled Components throughout?
  2. What's your vision for tests? Is there anything we should test with these individual components, or should we wait until we're making use of them?

@pvinis
Copy link
Contributor Author

pvinis commented Jun 23, 2020

I actually forgot about this pr 😅.

hm I didn't think of using styled. I guess we can.

For tests, since we are moving these to palette I think we could add some tests here later but I don't have a specific plan.

I'm ok with either merging and doing they changes in palette, or closing and doing everything directly in palette.

@mdole
Copy link
Contributor

mdole commented Jun 23, 2020

Ah yeah, and I forgot we're moving all of this to Palette haha. I'm going to reassign to you and let you make that call, both are totally reasonable to me and I'm not sure what your plan is for Palette so I don't want to slow you down! Let me know if I can help though

@mdole mdole assigned pvinis and unassigned mdole Jun 23, 2020
@pvinis pvinis requested review from mdole and ds300 July 8, 2020 15:36
@pvinis
Copy link
Contributor Author

pvinis commented Jul 8, 2020

Sorry for the many changes after upgrading this Pr from a draft to a regular one. This should be ready now.

@damassi
Copy link
Member

damassi commented Jul 9, 2020

@mdole - can this be merged?

@mdole
Copy link
Contributor

mdole commented Jul 9, 2020

@damassi ah sorry I keep getting distracted mid-review. I'll have a couple notes up in the next ~20min

Copy link
Contributor

@mdole mdole left a comment

Choose a reason for hiding this comment

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

couple minor things to quickly resolve, but overall this is looking great! Gratz on getting through some tough pagination issues @pvinis 🎉

@pvinis
Copy link
Contributor Author

pvinis commented Jul 13, 2020

With everything solved, I'll go ahead and mark this #mergeongreen.

@artsy-peril artsy-peril bot merged commit 0ad9ebc into master Jul 13, 2020
@artsy-peril artsy-peril bot deleted the viewing-room-list-ui branch July 13, 2020 10:22
@ds300
Copy link
Contributor

ds300 commented Jul 13, 2020

@pvinis for important new dependencies we normally talk about them in an RFC before adding them to eigen. I think relay-hooks definitely fits the bill. Do you mind submitting an RFC retroactively?

@pvinis
Copy link
Contributor Author

pvinis commented Jul 14, 2020

Oh sure. I thought it's small enough, as it's basically some hooks for relay that we already have.

@pvinis pvinis mentioned this pull request Jul 14, 2020
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants