-
Notifications
You must be signed in to change notification settings - Fork 2
Refactoring commits from WIP Android message table support PR #641
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
Conversation
✅ Deploy Preview for fxms-skylight ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
sarahhjchung
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had some comments about keeping TODO.md and the changes to launch.json in this PR. Also I noted down some tweaks I made to keep the desktop live message table looking the same as before.
.vscode/launch.json
Outdated
| "--runTestsByPath", | ||
| "${jest.testFile}" | ||
| ], | ||
| "cwd": "/Users/dmosedale/s/skylight", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this change be omitted from the commits? Seeing the cwd string made me curious if it should be or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, feel free to add a commit to reverse the entire launch.json change.
app/columns.tsx
Outdated
| // cell: (props: any) => { | ||
| // return SurfaceTag( | ||
| // props.row.original.template, | ||
| // props.row.original.surface, | ||
| // ); | ||
| // }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this PR, I'll leave this un-commented so that the desktop live message table looks as it does now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. We'll fix it up on the branch once we rebase there.
app/page.tsx
Outdated
| </div> | ||
| ); | ||
| export default function Page() { | ||
| return <Dashboard platform={"desktop"} />; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Capitalizing "desktop" so that the title for the desktop live message table is properly capitalized.
| return <Dashboard platform={"desktop"} />; | |
| return <Dashboard platform={"Desktop"} />; |
TODO.md
Outdated
| @@ -0,0 +1,107 @@ | |||
| Goal: stand up mobile version of experiments/rollouts | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I delete this file for this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, and MOBILE-EPICs.md too, please.
dmose
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sarah actually reviewed the code; I'm the original author, but github requires someone other than the submitted to do the approval. So. r=dmose

This PR holds the commits from the first commit to commit 51e7302 in #627
EDIT: included commit 43fd27b as well due to build errors