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 rooms visual cleanup #3232

Merged
merged 14 commits into from
Apr 28, 2020
Merged

Viewing rooms visual cleanup #3232

merged 14 commits into from
Apr 28, 2020

Conversation

mdole
Copy link
Contributor

@mdole mdole commented Apr 23, 2020

Still a few things to go here, but it's a start!

cc @iskounen @ds300 I'll work on wrapping this + #3215 tomorrow!

Here's the list I'm working off of:

  • Check with Ash about testing behavior for deeplinking
  • Remove "Viewing Room" text from header
  • Move flatlist from statement component to toplevel
  • Convert artworks flatlist so that each artwork is its own section + change type
  • Make # artworks correctly plural/singular for artworks button + rail
  • Subsection 30px top padding
  • Get hovering button to be sticky all the time
  • Test out deeplinking behavior - does back button take us home? (If you have the app open already, it takes you back to the screen you were already on. Seems good)
  • Get hovering button to only appear once we've scrolled to a certain point
  • Header image extends into the horns (leaving this for a followup PR that focuses on a few header tweaks)

#trivial

@mdole mdole force-pushed the viewing-rooms-visual-cleanup branch 2 times, most recently from 6bbbe8c to 45eda72 Compare April 28, 2020 14:14
@mdole mdole changed the title [WIP] Viewing rooms visual cleanup Viewing rooms visual cleanup Apr 28, 2020
@mdole mdole requested a review from ds300 April 28, 2020 14:14
Copy link
Contributor

@ds300 ds300 left a comment

Choose a reason for hiding this comment

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

Looking good, I had a couple of suggestions but feel free to merge after addressing those

Comment on lines +24 to +25
const totalCount = props.viewingRoomArtworks.artworks! /* STRICTNESS_MIGRATION */
.totalCount! /* STRICTNESS_MIGRATION */
Copy link
Contributor

Choose a reason for hiding this comment

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

as a follow-up it would be good to make these non-nullable in MP!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good thought! added to my to-do list

src/lib/Scenes/ViewingRoom/ViewingRoom.tsx Outdated Show resolved Hide resolved
src/lib/Scenes/ViewingRoom/ViewingRoom.tsx Outdated Show resolved Hide resolved
src/lib/Scenes/ViewingRoom/ViewingRoom.tsx Outdated Show resolved Hide resolved
@mdole mdole force-pushed the viewing-rooms-visual-cleanup branch from 57df02a to c2bf5ee Compare April 28, 2020 19:22
@mdole mdole merged commit a77d3f8 into master Apr 28, 2020
@mdole mdole deleted the viewing-rooms-visual-cleanup branch April 28, 2020 19:27
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.

2 participants