-
Notifications
You must be signed in to change notification settings - Fork 4
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
Implementing a button for Presence in the 2de kamer #891
base: staging
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## staging #891 +/- ##
===========================================
+ Coverage 13.57% 13.66% +0.08%
===========================================
Files 439 452 +13
Lines 3115 3103 -12
===========================================
+ Hits 423 424 +1
+ Misses 2692 2679 -13 ☔ View full report in Codecov by Sentry. |
cool! I will review this soon. Either this weekend or at our meeting next week |
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.
Looking very good overall! Differences between this and board-room-presence are abstractable, but that doesn't mean we should. Often, it is better to not introduce abstraction when it does not serve a practical purpose other than reducing the amount of code. This code is more maintainable as it is, because abstraction would introduce complexity.
I have left some remarks, let me know when you've implemented them.
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 component is written in the old style, please follow the new octane component style. There are a lot of examples that use glimmer components already
specifically, you can take example from https://github.com/csvalpha/amber-ui/pull/635/files#diff-b264f9e9a3db1a61c4d3c4c38a3c454a93a33286c5d2b1e820513bc9d4986084 where I've rewritten the board-room-presence component
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.
please also write this template in octane style. Some quick rules (without regard for whether you followed these or not):
- use angle bracket syntax for inserting components
- refer to controller properties/functions with
this.
i.e.this.property
orthis.someFunction
- refer to arguments passed to this component using
@
. So, if the parent component has passed an argument calledsomeArgument
, then we can refer to it in this template with@someArgument
. - do not use the action helper anymore. see Refactor:
{{action}}
helper usage #635 and Refactor:{{action}}
helper usage, the sequel #896 for examples of what to write instead.
please thoroughly familiarize yourself with #429 and the tutorials/guides/resources mentioned there.
adds #845
There were 2 ways I was considering implementing it, making a new component or rewriting the original component so that both presences indicators could be taken care of with one controller. I decided to implement a second controller because there are multiple difference between the 2 presences indicators.
needs csvalpha/amber-api#449