Skip to content

Step 0: Measurement code #6

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Step 0: Measurement code #6

wants to merge 2 commits into from

Conversation

Xiltyn
Copy link
Collaborator

@Xiltyn Xiltyn commented Oct 4, 2022 •
edited
Loading

  1. Added measure service
  2. Hooked up measurement code for main tabs + App.tsx
  3. Added an unoptimised timer to the Exhibitions screen

mdjastrzebski
mdjastrzebski reviewed Oct 4, 2022
View reviewed changes
src/navigation/bottom-tab-navigator.tsx Outdated
@@ -15,7 +15,7 @@ export const BottomTabNavigator = () => {

return (
<Tab.Navigator
initialRouteName="Welcome"
initialRouteName="Exhibitions"
Copy link
Collaborator

@mdjastrzebski mdjastrzebski Oct 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Pls change the initial route also for the main branch

mdjastrzebski
mdjastrzebski reviewed Oct 4, 2022
View reviewed changes
src/navigation/bottom-tab-navigator.tsx Outdated
name="Welcome"
component={Welcome}
name="About"
component={About}
options={() => ({header: Noop})}
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the story with header: Noop?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Literaly just didn't wanna have the inline () => null so I made this Noop placeholder. Happy to change that if you think it's unclear or looks weird

mdjastrzebski
mdjastrzebski reviewed Oct 4, 2022
View reviewed changes
src/screens/About/About.tsx Outdated
const isDarkMode = currentMode === 'dark';

const backgroundStyle = {
backgroundColor: Colors[currentMode || defaultColorMode],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Colors object with just single key looks a bit weird; could these be like:

const lightTheme: Colors {
  text: "#fff",
  background: "#fff",
}

const darkTheme: Colors {
  text: "#fff",
  background: "#fff",
} 

export const  colors = {
  light: lightTheme,
  dark: darkTheme,
} as const;

mdjastrzebski
mdjastrzebski reviewed Oct 4, 2022
View reviewed changes
src/screens/About/About.tsx Outdated
import { measure } from '~utils/measure';
import {Container, Header, Paragraph, SubHeader} from './About.styled';

type Props = {};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Screens typically do not have props.

mdjastrzebski
mdjastrzebski reviewed Oct 4, 2022
View reviewed changes
src/screens/About/About.tsx Outdated

return (
<React.Profiler id="Tab:About" onRender={measure.onRender}>
<SafeAreaView style={backgroundStyle}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lets put this About screen, without React.Profiler also to main, so that we only add Profiler here.

BTW how about creating withProfiler HOC for step ze that would simply wrap the component, it would not cause reformats.

  //... as is
});

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll see if I can get the HOC in :)

mdjastrzebski
mdjastrzebski reviewed Oct 4, 2022
View reviewed changes
src/screens/About/About.tsx Outdated
backgroundColor={backgroundStyle.backgroundColor}
/>
<Container>
<Header color={isDarkMode ? Colors.light : Colors.dark}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we make dark mode in the following way everywhere

Suggested change
<Header color={isDarkMode ? Colors.light : Colors.dark}>
<Header color={colors[currentMode].text}>

(this chage is for main)

mdjastrzebski
mdjastrzebski reviewed Oct 4, 2022
View reviewed changes
src/screens/Artworks/Artworks.tsx Outdated
@@ -28,49 +29,51 @@ export const Artworks = ({}: Props) => {
const backgroundStyle = {
backgroundColor: Colors[currentMode || defaultColorMode],
};
const queryOptions = {limit: '15'};
const queryOptions = {limit: '50'};
Copy link
Collaborator

Choose a reason for hiding this comment

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

move also to main

mdjastrzebski
mdjastrzebski reviewed Oct 4, 2022
View reviewed changes
src/screens/Exhibitions/Exhibitions.tsx Outdated
const currentMode = useColorScheme();
const isDarkMode = currentMode === 'dark';

const getTimerLabel = () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const getTimerLabel = () => {
const formatTimeLeft = () => {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

🚀

mdjastrzebski
mdjastrzebski reviewed Oct 4, 2022
View reviewed changes
src/screens/Exhibitions/Exhibitions.tsx Outdated

React.useEffect(() => {
timer.current = setInterval(() => setTimerLabel(getTimerLabel()), 1000);
}, [getTimerLabel]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

return () => clearInterval(timer.current)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

<3

mdjastrzebski
mdjastrzebski reviewed Oct 4, 2022
View reviewed changes
Copy link
Collaborator

@mdjastrzebski mdjastrzebski left a comment

Choose a reason for hiding this comment

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

Looks good. Kuba pls move all of the code, except adding Measure tools and React.Profiler also to main branch. I plan to have step 0 just to add measuring code.

All other changes, like page size, tab order, etc should be moved to main

@Xiltyn Xiltyn force-pushed the step-0_measurementc-code branch 2 times, most recently from 195801f to f7c70b7 Compare October 4, 2022 21:36
@Xiltyn
Copy link
Collaborator Author

Xiltyn commented Oct 4, 2022

Updated based on comments and rebased with current state of the main branch. Please review @mdjastrzebski

@Xiltyn Xiltyn requested a review from mdjastrzebski October 4, 2022 21:38
@Xiltyn Xiltyn force-pushed the step-0_measurementc-code branch from 3fedc27 to c90e705 Compare October 5, 2022 09:42
@Xiltyn Xiltyn changed the title Step 0 measurement code Step 0: Measurement code Oct 5, 2022
@mdjastrzebski mdjastrzebski force-pushed the step-0_measurementc-code branch from c90e705 to 1e4db4c Compare October 5, 2022 11:37
@Xiltyn Xiltyn force-pushed the step-0_measurementc-code branch from 826aa56 to cdf973e Compare October 5, 2022 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdjastrzebski mdjastrzebski mdjastrzebski approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Xiltyn @mdjastrzebski @hurali97