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

New Design #1503

Closed
wants to merge 34 commits into from
Closed

New Design #1503

wants to merge 34 commits into from

Conversation

dragos-efy
Copy link

@dragos-efy dragos-efy commented Jul 8, 2021


New Design

Description
Integration with the Design-Tweaks fork

Pull Request Type

  • Bugfix
  • Feature Implementation

Screenshots

Related issue
closes #1452

@ChunkyProgrammer
Copy link
Member

ChunkyProgrammer commented Jul 8, 2021

Exclude changes to workflow (yml files), package.json & the ReadMe?

@dragos-efy
Copy link
Author

Of course haha! It's not the final edit and lots of parts can't even be integrated yet. @GilgusMaximus suggested I should make a new branch and PR and make edits, so that's why. Sorry for the confusion lol

@efb4f5ff-1298-471a-8973-3d47447115dc

Q1: will there be a toggle to switch to rounded corners or the default design in the settings?

Q2: Will there be changes that are not theme based? So these will effect the whole application doenst matter which theme u select.

@dragos-efy
Copy link
Author

A1: I proposed that but I'm not in control of the decision... only my own fork, so that depends on what others want.
A2. Technically yes, for example the width of the video gallery, which is larger now. But this pr has lots of new changes... so I don't know which ones will or will not be implemented.

@efb4f5ff-1298-471a-8973-3d47447115dc

This PR is getting blocked by this one #1461 right?

@dragos-efy
Copy link
Author

yea, this one basically contains that one lol

@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

efb4f5ff-1298-471a-8973-3d47447115dc commented Jul 10, 2021

If this issue also contains it, i think its better to close that other one.

@dragos-efy
Copy link
Author

closed

@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

efb4f5ff-1298-471a-8973-3d47447115dc commented Jul 11, 2021

Btw u have to add "closes #1452"

After this PR is merged the issue will close automatically instead of staying open and clutter the backlog.

@dragos-efy
Copy link
Author

Thank you! I added that and now it says it will be closed if this gets merged

@peepo5
Copy link
Contributor

peepo5 commented Aug 23, 2021

Give a rundown of what has changed in this pr

@peepo5
Copy link
Contributor

peepo5 commented Aug 23, 2021

Also have been looking at your version. I really think that your changes should be split into several different pr's. Adding a monolith pull request like this one complicates and makes it hard to review all the changes. Also your version seems very experimental. I think moving changes slowly through seperate non-conflicting pr's would be a better idea.

@dragos-efy
Copy link
Author

dragos-efy commented Aug 26, 2021

Splitting things & Broken implementation

I realized that the localstorage doesn't have a default, so for people using my edits for the first time it's broken af (it's easily fixable tho), but I haven't had time to take care of it and this week I still won't have time... Most likely next week as well, but I'll fit it in my schedule if I can. About splitting them... I understand your point, but some changes have a completely new approach like ignoring all current css color variables, adding new ones (which if it gets merged the old ones would need to be removed so that there's no variables that won't be used at all. The logo uses dynamic variables inside svg as well so that it changes its colour... Quite a few moving parts and separating them would make them not work on their own... Plus the video grid idea I had was a bad implementation, because it combines videos with playlists if you go to a channel page so that needs a retake urgently.

Colours & Radius + Broken Video/Thumbnail Grid

If this was split I'd split it into colours and border radius (including their variables) as a category and then other stuff as other categories... but for now the video/thumbnail grid should definitely be separated and ignored from the whole implementation, unless I have time to fix it or someone fixes it... Also, I don't know how to change and add settings in the settings page, which is why I created my own menu menu that's controlled by javascript... so if someone could put those settings I have in my menu inside the settings page, that would be cool.

1 vs 2 main colours

And lastly with colours, in my version there's 1 main colour (which is much easier to manage and more consistent if you ask me, but people have the right to voice their opinion, so... haha), but in case people get pissed and desperately want 2 main colours, there should be 2 colour pickers instead of 1 with new variables...

Better comunication & organization

If possible, in the future I'll try communicating more with devs that know how nodejs, vue, electron, ejs work and if they're down let them guide that technical part, but combine it with my design ideas for things I'm good at, cuz my weakness on this project is not knowing those 4 frameworks, leading me to "hacking" the app through javascript.

What should I do?

I'm also confused with what I should open or not cuz I've been told to close things, so decide on that and I'll help with what I can depending on how my time allows it, as I said it's a very busy period for me.

@dragos-efy
Copy link
Author

dragos-efy commented Aug 26, 2021

oh and in terms of what I changed... lots of things, I even forgot what I did, but in the ejs file if you look at the css part there's comments I made for each category, like

/* Video Player */ someclass {somevalue}
/* Video Grid */ someclass {somevalue}
/* Border Radius */ someclass {somevalue}
/* Light, dark, black mode */ someclass {somevalue}

It should be pretty simple to tell because inside of the ejs file I put both the css and javscript as <script> and <style> tags and gave them comments for everything to keep them sorta organized.

@kommunarr
Copy link
Collaborator

I think if you can produce an up-to-date changelog for every area of the app (including what features/changes are added/reworked, what features are removed, what's breaking now that didn't before), this would be much easier to review and discuss. There seem to be a ton of good changes in this, but it is just good to know which exact things are not working so they can be fixed or rolled back as appropriate.

@dragos-efy
Copy link
Author

Alright

@peepo5
Copy link
Contributor

peepo5 commented Aug 28, 2021

Agreed

@PrestonN
Copy link
Member

PrestonN commented Sep 1, 2021

Hey there @dragosnfy. I know we've talked a bit about these design changes in our Matrix channel. While I'm still interested in possibly implementing some of these changes, I'm going to go ahead and close this PR for now.

Looking through your changes, it looks like they were made outside of our existing Vue component structure and also includes your changes made specifically for the fork you've created. The changes made for your specific fork will need to be removed and your existing code will need to be reworked to be inside of our components in order for us to be able to merge this. This may take a while which is why I've decided to close this. You are more than welcome to create a new PR once these changes are ready.

Myself as well as some of the other contributors will be more than happy to help lead you in the right direction for doing this properly. You're already active in our Matrix channel so you already know where to find us. During the process, we can maybe break down this PR into smaller sections so that we can make sure everything is implemented properly. The other issue with PRs this large is that it can disrupt the other PRs currently out there and can force other contributors to restructure their PR because of the large changes that we've included. Breaking this down can help prevent that as well.

I hope you understand my decision with this and I don't want to sound too harsh about it. The designs you did are genuinely good and I'd like to look at them more, however we'll have to do a lot before they'll be ready to include upstream. Like I said, I'm available to help you with this process if you'd like.

@PrestonN PrestonN closed this Sep 1, 2021
@dragos-efy
Copy link
Author

Nah, you don't sound harsh, don't worry about that. In regards to breaking things for other contributors, I'd rather break things massively once than multiple times, like GTK4 did, but I'm fine with whatever you decide, I just made public the codes I was already using for myself and even on my fork I said I made it for things that most likely won't be merged, so I wasn't expecting an interest in it. I would've done about the same stuff if it wasn't for others seeing my tweaks cuz I'm obsessed with making the things I and others use cool. As for talking, I'll ask you questions once I get more time to actually do something. There's a lot of reorganizing with my company and clients rn and that takes a lot of time, but once things are more stable, I'll get back to this and most likely obsessively fix everything in a week or something haha

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.

Add option for rounded grid elements within app
6 participants