-
-
Notifications
You must be signed in to change notification settings - Fork 172
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
Adding feature star rating to top ribbon #562 #586
base: main
Are you sure you want to change the base?
Conversation
Video name column and star rating column overlapped due to absolute position -> fixed TC (top component) star rating shows rating from sheet's rating -> fixed TC star rating is unable to update after sheet's rating changes, need to click on video again to update -> WIP TC star rating is unable to update directly on TC, due to index error -> WIP
Really cool! Thank you 🙇 I'll test it out today or this weekend and let you know if anything needs changing -- by looking at the code, looks good! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really cool! It works 😁
I am recommending that the star rating template, css, and component code all becomes part of its own component - so we can just re-use it in both places 👍
Please let me know if you would like any guidance or help -- any time! 🤝
@@ -77,6 +73,33 @@ | |||
} | |||
} | |||
|
|||
.star-rating { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is duplicating the CSS that we already have.
In general it's better to have both locations re-use the same CSS - so that we only need to update it in one place (and the visual style is updated in both locations).
One approach is to take out the star-rating
class and put it in its own file, and have both templates point to it.
But it really feels best to just break out the star rating chunk into its own element 👍
@@ -52,4 +76,16 @@ export class TopComponent { | |||
this.onOpenInExplorer.emit(true); | |||
} | |||
|
|||
setStarRating(rating: StarRating): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is duplicating setStarRating
in the meta-component
- so it's a good time to create a new component that will have this functionality 👍 and we can then remove this method from both here and meta-component
👍
{{ item }} | ||
</span> | ||
</div> | ||
<button |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section is duplicating the same code as happens in the meta-component
so it would be best to just create a new component which will have this chunk in its template, and the setStarRating
method in its component.ts
file 👍
@@ -48,7 +48,7 @@ | |||
"trash": "6.1.1" | |||
}, | |||
"devDependencies": { | |||
"@angular-devkit/build-angular": "0.1000.0", | |||
"@angular-devkit/build-angular": "^0.1100.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind too much, but it's generally better not to change versions and keep a PR focused on one thing.
We could update to Angular 11 in a separate PR 🤝
If there's going to be a details tab on the bottom tray, I feel that would definitely be better than having the star rating component on the top component. Especially since users would be able to edit and makes changes to the video details a lot easier that way. 👍 I did make changes to separate the star rating component into its own component. It may help with future changes (or not 😅 ). Feel free to discard the changes (I know this issue has been open for a long time so you probably considered other ideas as well). I learned a lot, and I'm glad I found this project (first time working with Angular and Electron). I will continue to look out for other issues 😄 |
This is my first time contributing. Please let me know if there are any issues or concerns, and I will try my best to fix :)