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

Fix: Issue #98 #166

Closed
wants to merge 4 commits into from
Closed

Fix: Issue #98 #166

wants to merge 4 commits into from

Conversation

teamomiamigo
Copy link
Collaborator

What needs to be changed:
Changing the text in the search bar of the map to be white during dark mode and not be black

Why is changed:
The text is unreadable during dark mode as the text is in black, where it should be white and be applied with the theme color.

How it is changed:
Whether in dark mode in light mode, I have ensured that the text color represents the dark or light theme

Copy link
Collaborator

@AmarHadzic AmarHadzic left a comment

Choose a reason for hiding this comment

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

Overall nice work, very simple and clean solution

@@ -649,12 +649,12 @@ const styles = StyleSheet.create({
position: "absolute",
marginTop: Constants.statusBarHeight,
flexDirection: "row",
backgroundColor: "#fff",
backgroundColor: theme.primaryColor,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice job making it a variable

width: "90%",
alignSelf: "center",
borderRadius: 5,
padding: 10,
shadowColor: "#ccc",
shadowColor: theme.text,
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice work switching from a hard coded value to a variable

@@ -716,6 +716,7 @@ const styles = StyleSheet.create({
textContent: {
flex: 2,
padding: 10,
color: theme.text,
},
cardtitle: {
fontSize: 12,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a lot of lines for a file, do we need it to be this long?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe look to divi up some of these components into more files

@AmarHadzic AmarHadzic requested review from AmarHadzic and rcAsironman and removed request for AmarHadzic September 16, 2024 22:16

Choose a reason for hiding this comment

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

What is the purpose of these changes? Looks like rearranging of imports.

import { formatToLocalDateString } from "../../components/time";
import ApiService from "../../utils/api_calls";
import NoteDetailModal from "./NoteDetailModal";
import { mapDarkStyle, mapStandardStyle } from "./mapData";

Choose a reason for hiding this comment

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

These changes look like rearrangement of imports, is there a purpose behind this?

@rcAsironman
Copy link
Collaborator

Hello @teamomiamigo,

I appreciate your efforts, but I’m encountering issues with the code. I’m unable to simulate the app, and it throws a "theme doesn't exist" error.

Additionally, when raising a PR, please include a comment in the PR description indicating the issue it resolves. For example, if the PR addresses issue #160, the first comment in the PR should be: "Fixes #160."

Thank you for your attention to these matters.

WhatsApp Image 2024-09-18 at 6 28 38 PM

@teamomiamigo
Copy link
Collaborator Author

I have made new commits and changes to fix the colors so that the search bar text is in the black text and is set to the white text during dark mode now. due to simulator issues, I wasn't able to see this issue in this.

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.

5 participants