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: Revert Vite addition + Apps now have correct themes #997

Merged
merged 14 commits into from
May 27, 2023

Conversation

Mycroft-Studios
Copy link
Contributor

@Mycroft-Studios Mycroft-Studios commented Apr 26, 2023

Adding vite, completely broke the phone.
Apps also didnt seem to follow the themes correctly - text would be pure black, on dark mode.

Before:
image

After:
image

Pull Request Checklist:

  • Have you followed the guidelines in our Contributing document and Code of Conduct?
  • [x ] Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • [x ] Have you built and tested NPWD in-game after the relevant change?

@itschip @LiamDormon

Copy link
Collaborator

@AvarianKnight AvarianKnight left a comment

Choose a reason for hiding this comment

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

Only real issue I see is using phone_name instead of phoneName

@Mycroft-Studios
Copy link
Contributor Author

Only real issue I see is using phone_name instead of phoneName

Fixed :)

Copy link
Collaborator

@AvarianKnight AvarianKnight left a comment

Choose a reason for hiding this comment

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

LGTM will wait on chips review though before merge :)

@@ -19,10 +19,10 @@ export const SearchContacts: React.FC = () => {

return (
<div className="w-full py-2 px-4">
<div className="flex items-center justify-start space-x-2 rounded-md bg-gray-200 px-2 dark:bg-neutral-800">
<div className="flex items-center justify-start bg-neutral-200 dark:bg-neutral-800 rounded-md px-2 space-x-2">
Copy link
Contributor

Choose a reason for hiding this comment

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

why tailwind ... ACK REJECTED

jk LGTM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's what happens when you revert broken commits 😅

@@ -43,6 +45,7 @@ interface SoundItemProps {
icon: JSX.Element;
tooltip: string;
onPreviewClicked: any;
theme: any;
Copy link
Contributor

Choose a reason for hiding this comment

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

Theme ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

theme

@@ -110,6 +116,7 @@ interface SettingSwitchProps {
onClick: any;
icon: JSX.Element;
secondary: string;
theme: any;
Copy link
Contributor

Choose a reason for hiding this comment

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

D:

@@ -163,7 +163,7 @@ export const SettingsApp: React.FC = () => {

const [openMenu, closeMenu, ContextMenu, isMenuOpen] = useContextMenu();
const classes = useStyles();

const theme = useTheme();
Copy link
Contributor

@jfrader jfrader May 5, 2023

Choose a reason for hiding this comment

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

Why passing theme as prop EVERYWHRE instead of using useTheme when needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Except, I am using it when needed....
Anywhere i put it, it's required for the phone to work correctly with themes

@@ -8,7 +8,7 @@
"types": "dist/index.d.ts",
"scripts": {
"test": "echo \"Error: no test specified\" && exit 1",
"dev:old": "tsup src/index.ts --watch --dts --format esm,cjs"
Copy link
Contributor

Choose a reason for hiding this comment

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

nice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's what happens when you revert broken commits 😅

@jfrader
Copy link
Contributor

jfrader commented May 5, 2023

I'm bored and miss npwd sometimes 💋

@@ -8,6 +8,7 @@ import { useSettingsValue } from '../../../apps/settings/hooks/useSettings';
import { IconSetObject } from '@typings/settings';
import { useRecoilValue } from 'recoil';
import { phoneState } from '@os/phone/hooks/state';
import { extname } from 'path';
Copy link
Member

Choose a reason for hiding this comment

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

to remove?

@itschip
Copy link
Member

itschip commented May 8, 2023

@Mycroft-Studios looks like lockfile is outdated from the CI. Try installing deps locally, and see if you'll get any changes locally. If not just delete, generate it, and then push the changes.

@Mycroft-Studios
Copy link
Contributor Author

@Mycroft-Studios looks like lockfile is outdated from the CI. Try installing deps locally, and see if you'll get any changes locally. If not just delete, generate it, and then push the changes.

will do :)

@TonybynMp4
Copy link
Contributor

@Mycroft-Studios looks like lockfile is outdated from the CI. Try installing deps locally, and see if you'll get any changes locally. If not just delete, generate it, and then push the changes.

will do :)

do 🔫

@GlitchOo
Copy link
Contributor

Should be updated in PR #1004

@itschip itschip merged commit 134372c into project-error:master May 27, 2023
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.

6 participants