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

Port upstream commits into lavender #9

Merged
merged 13 commits into from
Dec 27, 2024
Merged

Conversation

tulpenkiste
Copy link
Member

@tulpenkiste tulpenkiste commented Dec 19, 2024

TODO

  • Ensure everything works
  • Revert unwanted changes (idk if all of them are gone'd)
  • Fix friend requests

@catuhana
Copy link

I was going to create a review but there are so many changes so it's better to comment - in many (probably every) files, branding, URLs, terms (space vs server) seems to be reverted.

@tulpenkiste
Copy link
Member Author

I was going to create a review but there are so many changes so it's better to comment - in many (probably every) files, branding, URLs, terms (space vs server) seems to be reverted.

Yeah I'm working on it

@tulpenkiste
Copy link
Member Author

Added a friend request fix but needs an additional patch on upryzing.js

@tulpenkiste
Copy link
Member Author

tulpenkiste commented Dec 19, 2024

Coauthor list to add when squash merging:

Revolt CI <[email protected]>
Paul Makles <[email protected]>
Mikhail Kobik <[email protected]>
JackDotJS <[email protected]>
Kirill Mironov <[email protected]>
Rizon <[email protected]>
2t34lth <[email protected]>

@tulpenkiste tulpenkiste marked this pull request as ready for review December 27, 2024 00:16
@tulpenkiste tulpenkiste changed the title WIP: Port upstream commits into lavender Port upstream commits into lavender Dec 27, 2024
@tulpenkiste
Copy link
Member Author

Just to note, this will be manually merged (as to add coauthors in a squash commit)

@tulpenkiste
Copy link
Member Author

Just to note, this will be manually merged (as to add coauthors in a squash commit)

Nevermind, turns out I can squash and add coauthors inside of GitHub. Not sure why this isn't documented but ok github

Copy link
Member

@amycatgirl amycatgirl left a comment

Choose a reason for hiding this comment

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

Review full of nits

Although I am noticing a bunch of changes that override existing commits, so those need to be fixed before we merge these.

@@ -23,7 +23,7 @@ cd lavender
# git submodule init && git submodule update

# install all packages
pnpm i
pnpm i --frozen-lockfile
Copy link
Member

Choose a reason for hiding this comment

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

Why is --frozen-lockfile here?

When developing, it's important that we keep the lockfile up to date, --frozen-lockfile should only be used in CI/CD environments (imo)

Copy link
Member Author

Choose a reason for hiding this comment

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

I have 0 clue, ask the guy who put it on upstream

Copy link
Member Author

Choose a reason for hiding this comment

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

Although I think its less for developers and more for users that just want newer commits

Copy link
Member

Choose a reason for hiding this comment

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

Hm, maybe.

Might add a disclaimer on the README then

@@ -55,7 +55,7 @@ Any edits to the `uprzying.js` codebase will immediately be reflected while deve

```bash
# install packages
pnpm i
pnpm i --frozen-lockfile
Copy link
Member

Choose a reason for hiding this comment

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

Refer to my previous comment

@@ -109,7 +109,7 @@ export default function ServerOverview(props: ServerSettingsProps) {
</FormGroup>
<FormGroup>
<CategoryButton
description="Server can be joined from Discover"
description="Space can be joined from Discover"
Copy link
Member

Choose a reason for hiding this comment

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

TODO: i18n

@@ -28,7 +28,7 @@ const Config: SettingsConfiguration<Server> = {
title(key) {
const t = useTranslation();
return t(
`app.settings.server_pages.${key.replaceAll("/", ".")}.title`,
`app.settings.server_pages.${key.replaceAll("/", ".")}.title` as any,
Copy link
Member

Choose a reason for hiding this comment

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

Um, doesn't eslint complain about using any here?

Either way, the proper type here should be string

Copy link
Member Author

Choose a reason for hiding this comment

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

Its either eslint warning or TS error

@@ -62,7 +62,9 @@ export default function Keybinds() {

const translateCombo = (combo: KeyCombo, short: boolean) =>
combo
.map((key) => t(`keys.${key}.${short ? "short" : "full"}`, {}, key))
.map((key) =>
t(`keys.${key}.${short ? "short" : "full"}` as any, {}, key)
Copy link
Member

Choose a reason for hiding this comment

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

Again, string type instead of any.

})
}
>
OK
Copy link
Member

Choose a reason for hiding this comment

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

TODO: i18n

@@ -29,7 +29,8 @@ export function ConversationStart(props: Props) {
</Show>
<Typography variant="conversation-start">
{t(
`app.main.channel.start.${props.channel.type === "SavedMessages" ? "saved" : "group"
`app.main.channel.start.${
Copy link
Member

Choose a reason for hiding this comment

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

oh and HERE it doesn't cast to any

awesome
so cool /s

@@ -48,7 +48,7 @@ export default {
grow: true,
},
decorators: [
(props) => (
(props: any) => (
Copy link
Member

Choose a reason for hiding this comment

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

Im going to let this slide just because i have no clue what type props should be

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it has a type

Copy link
Member

Choose a reason for hiding this comment

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

huh

i guess i'll check it later

Copy link
Member

Choose a reason for hiding this comment

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

These are going to be replaced again anyway so 🥴

Copy link
Member

Choose a reason for hiding this comment

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

um

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't look at me, I've got no clue

Copy link
Member

@amycatgirl amycatgirl left a comment

Choose a reason for hiding this comment

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

yay

@tulpenkiste
Copy link
Member Author

yay

air-jail-cat

@amycatgirl
Copy link
Member

😭😭😭😭😭😭😭😭😭😭😭😭😭😭😭

@tulpenkiste tulpenkiste merged commit 1b59e71 into main Dec 27, 2024
3 checks passed
@tulpenkiste tulpenkiste deleted the mygodwhatareyoudoing branch December 27, 2024 19:05
Copy link
Member

@Rexogamer Rexogamer left a comment

Choose a reason for hiding this comment

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

I didn't get around to reviewing the whole thing, apologies, but this is what I did spot. GitHub is lagging severely, blocking me from checking each file, so apologies if you already fixed these

Comment on lines -176 to -180
<Match when={props.message.member?.timeout}>
<Tooltip content={`User has been timed out for ${dayjs(props.message.member?.timeout).toNow(true)}`} placement="top">
<MdSchedule {...iconSize(16)} />
</Tooltip>
</Match>
Copy link
Member

Choose a reason for hiding this comment

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

you seem to have accidentally removed the timeout indicator

Comment on lines -182 to +187
icon: <MdSmartToy fill={theme!.colours.foreground} {...iconSize(20)} />,
icon: <MdSmartToy {...iconSize(20)} />,
title: t("app.settings.pages.bots.title"),
},
{
id: "feedback",
icon: <MdRateReview fill={theme!.colours.foreground} {...iconSize(20)} />,
icon: <MdRateReview {...iconSize(20)} />,
Copy link
Member

Choose a reason for hiding this comment

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

this needs to be re-added for dark mode support

Comment on lines -270 to +261
icon: <MdMemory fill={theme!.colours.foreground} {...iconSize(20)} />,
icon: <MdMemory {...iconSize(20)} />,
Copy link
Member

Choose a reason for hiding this comment

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

ditto

@@ -18,8 +18,6 @@ import MdFace from "@material-design-icons/svg/outlined/face.svg?component-solid
import MdPersonAddAlt from "@material-design-icons/svg/outlined/person_add_alt.svg?component-solid";
Copy link
Member

Choose a reason for hiding this comment

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

re-add timeout stuff in this file (see below removals)

<span>your way.</span>
</b>
<span style={{ "text-align": "center", opacity: "0.5" }}>
Connect with Upryzing.
Copy link
Member

Choose a reason for hiding this comment

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

the text was here

}}
>
<span>Your conversations,</span>
<br />
Copy link
Member

Choose a reason for hiding this comment

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

this was removed for leaving a weird gap


/**
* Load and set a language by the given key
* set a language by the given key
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* set a language by the given key
* Set a language by the given key

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Merged
Development

Successfully merging this pull request may close these issues.

4 participants