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

About page and fixing some ESLint problems by import react package #3167

Merged
merged 1 commit into from
Mar 18, 2022

Conversation

tpmai22
Copy link
Contributor

@tpmai22 tpmai22 commented Mar 10, 2022

Issue This PR Addresses

Fixed #2859

Type of Change

  • Bugfix: Change which fixes an issue
  • New Feature: Change which adds functionality
  • Documentation Update: Change which improves documentation
  • UI: Change which improves UI

Description

Add the About page to our mobile app

Steps to test the PR

  1. Open my PR in Gitpod
  2. cd src/mobile
  3. Run pnpm start
  4. Refer to mobile docs on choosing testing device

Checklist

  • Quality: This PR builds and passes our npm test and works locally
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not (if applicable)
  • Documentation: This PR includes updated/added documentation to user exposed functionality or configuration variables are added/changed or an explanation of why it does not(if applicable)
Android.Emulator.-.Pixel_4_API_32_5554.2022-03-11.10-16-44.mp4

@gitpod-io
Copy link

gitpod-io bot commented Mar 10, 2022

src/mobile/package.json Outdated Show resolved Hide resolved
src/mobile/src/screens/AboutScreen.jsx Outdated Show resolved Hide resolved
src/mobile/src/screens/AboutScreen.jsx Outdated Show resolved Hide resolved
src/mobile/src/screens/AboutScreen.jsx Show resolved Hide resolved
alignItems: 'center',
},
scrollView: {
width: '80%',
Copy link
Contributor

Choose a reason for hiding this comment

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

how about 90% - 95%, there is some unnecessary padding

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just feel like this will be a bit delegant from my pov but I need more though on this @Andrewnt219

src/mobile/src/screens/AboutScreen.jsx Show resolved Hide resolved
@TueeNguyen
Copy link
Contributor

Nit: the scroll bar could be more transparent
image

@tpmai22
Copy link
Contributor Author

tpmai22 commented Mar 11, 2022

Fixing most of the stuff except the margin and update on how it look in the description

@TueeNguyen
Copy link
Contributor

The brightness in the middle makes my eyes hurt :)
image

@tpmai22
Copy link
Contributor Author

tpmai22 commented Mar 11, 2022

The brightness in the middle makes my eyes hurt :) image

I try to be fancy but I guess I will keep it simple from now

tcvan0707
tcvan0707 previously approved these changes Mar 11, 2022
Copy link
Contributor

@Kevan-Y Kevan-Y left a comment

Choose a reason for hiding this comment

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

Having some marginBottom here would be good.
image

src/mobile/package.json Outdated Show resolved Hide resolved
@@ -1,3 +1,4 @@
import React from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought we had a fix before to allow us not having ESlint to show an error when react not imported

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is still so I wonder

@tpmai22 tpmai22 requested a review from tcvan0707 March 11, 2022 17:19
@TueeNguyen
Copy link
Contributor

TueeNguyen commented Mar 11, 2022

Looks ok to me, needs another rebase @tpmai22

@nguyenhung15913
Copy link
Contributor

Great job !!
But I wonder should we use the original design of text instead:
image

@Kevan-Y
Copy link
Contributor

Kevan-Y commented Mar 15, 2022

Great job !! But I wonder should we use the original design of text instead: image

Nice catch probably we should. It can be done in a follow-up issue. Since this has been sitting here for a long period and is close to being merged.

@Kevan-Y
Copy link
Contributor

Kevan-Y commented Mar 15, 2022

@tpmai22 Could you rebase your branch

Kevan-Y
Kevan-Y previously approved these changes Mar 17, 2022
- stop() now return Promise
- Add some fixes to tests
- Add shutdown() functions to Satellite.stop()
- Change Elastic mock to reinitialize beforeEach
@tpmai22
Copy link
Contributor Author

tpmai22 commented Mar 18, 2022

@rclee91 we can merge this

@cindyorangis cindyorangis dismissed stale reviews from nguyenhung15913 and Kevan-Y via 0d072ca March 18, 2022 19:56
@Kevan-Y
Copy link
Contributor

Kevan-Y commented Mar 18, 2022

Yeah a rebase, and can be merged. Create a follow issue to apply font-family to all the screen

@RC-Lee
Copy link
Contributor

RC-Lee commented Mar 18, 2022

Waiting for tests to rerun, since it failed the previously

@RC-Lee RC-Lee merged commit 505ed86 into Seneca-CDOT:master Mar 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

React Native - About Page
6 participants