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

Support For Vanity Urls #507

Merged
merged 11 commits into from
Aug 14, 2023
Merged

Support For Vanity Urls #507

merged 11 commits into from
Aug 14, 2023

Conversation

SrS2225a
Copy link
Contributor

Allows you (the user) to rename the random ID that uploads are assigned and give them a special vanity/custom url! As example: If XBackBone generated a link such as https://media.nyxgoddess.org/talI2/DIsexobi55.mp4, but want something a little more custom, you can easily change that link to https://media.nyxgoddess.org/talI2/amongus.mp4.

Additionally, if you try to change the generated link to a invalid url such as "https://media.nyxgoddess.org/talI2/among us.mp4" XBackBone will automatically add a - to make it valid. Like so: https://media.nyxgoddess.org/talI2/among-us.mp4

@SrS2225a
Copy link
Contributor Author

Oh no, I see. Should I worry about these failed tests and clean up those code quality issues before you merge it? Or will the current changes be sufficient enough?

@sergix44 sergix44 self-requested a review December 29, 2022 16:15
@sergix44
Copy link
Owner

Oh no, I see. Should I worry about these failed tests and clean up those code quality issues before you merge it? Or will the current changes be sufficient enough?

Yes please try to make the CI pass, at least the stuff related to code style 🙏

@SrS2225a
Copy link
Contributor Author

Hi, I have fixed those styling issues and the CI was able to pass as normal. Do you need anything else?

@sergix44
Copy link
Owner

Hi, I have fixed those styling issues and the CI was able to pass as normal. Do you need anything else?

Thanks! I'll review the PR asap

@SrS2225a
Copy link
Contributor Author

Hi,

It's been a while since I heard anything from this PR so I wanted to see if you were able to review it and everything was alright. Please let me know if you have any questions or want anything changed before you accept it

Copy link
Owner

@sergix44 sergix44 left a comment

Choose a reason for hiding this comment

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

Nice start! There are some things that should be considered tho

app/Controllers/MediaController.php Outdated Show resolved Hide resolved
app/Controllers/MediaController.php Outdated Show resolved Hide resolved
app/Controllers/MediaController.php Outdated Show resolved Hide resolved
app/Controllers/MediaController.php Show resolved Hide resolved
src/js/app.js Outdated Show resolved Hide resolved
resources/templates/comp/modal_vanity.twig Outdated Show resolved Hide resolved
@SrS2225a
Copy link
Contributor Author

SrS2225a commented Jan 14, 2023

Thanks, glad you like the PR! Sorry about the issues tho. I will start working on solving all of the requested fixes. I will add a comment when it is complete, or when I have any other further comments starting out from the easiest one to fix to the requested changes. Once all of them are fixed I will add a separate comment letting you know

@SrS2225a
Copy link
Contributor Author

Hi,

I have fixed all of the issues as you requested and pushed the changes to my fork. We should all be good to go. Please let me know if I missed anything. I also added comments in reply to your feedback on my thoughts.

@SrS2225a SrS2225a requested a review from sergix44 February 4, 2023 05:22
@SrS2225a
Copy link
Contributor Author

SrS2225a commented Feb 8, 2023

Hello,

I wanted to see how the review was going as I have not heard anything from you for this merge request in a while. Is everything alright? I want to make sure everything was good with you and have not missed anything so this feature becomes official!

@Jeyso215
Copy link

@sergix44 Please respond, I know you got real life stuff you got going on.

@sergix44
Copy link
Owner

I'll check this out soon, sorry for the late reply, the notification was lost in the ddos that I receive every day

src/js/app.js Outdated Show resolved Hide resolved
resources/templates/dashboard/grid.twig Outdated Show resolved Hide resolved
resources/templates/dashboard/grid.twig Outdated Show resolved Hide resolved
@DinnerBeef
Copy link

@SrS2225a any idea when you will get a chance to look at this? Been hoping this gets merged in for a while.

@SrS2225a
Copy link
Contributor Author

Ill be taking a look now. Been too busy with personal matters over the last month, so I have not been table to take a look for a while

@sergix44 sergix44 merged commit 7f07ecd into sergix44:master Aug 14, 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.

4 participants