Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
UT-210: Rebuilding of the code structure for Cliptool. #81
UT-210: Rebuilding of the code structure for Cliptool. #81
Changes from 17 commits
a371ad3
3768d71
bc469b5
03ba501
d478cfd
8979edd
17dbde7
8bd7272
ecd3750
ec86a3d
f5f07e7
e291515
0890dd4
65b597d
982c6d0
216dd81
547aa5c
cbcfc32
1e69ed3
cb11042
e5d9f3b
623091c
4fa367f
457aa0d
177dfcf
ee6bcb3
c0b6131
f4e892d
bb89259
6d5ed12
f5b36e1
ebc9778
5a9fbf6
60ab564
463ea9a
00ddfa6
bb9b9e4
2911e3a
8e3e320
3f26b81
b7e716e
092b934
3b30b08
a48517b
83fca71
a689c39
2f84be2
0c63522
c5dca7c
5de294d
cee5bac
91e9913
d315cb1
2f1e70f
f013b07
cd6775f
c45d24b
bc0b448
8bbba23
978e3d1
f04a6ba
f1dcd1c
39090d9
8637cd1
4a43b0a
a44fc8e
e490351
9874d9f
c83ae54
e89871c
bc5d0d4
60a078f
75df749
10aefbd
e22d683
20afcc7
775eea4
bf1ff27
61e3216
b22c44a
7ae2d08
8c6f8b7
38d128a
8d211c5
1118a0d
ed8f564
a8cea34
b36c3f2
33ca811
d3b3d0b
1cf582c
7ae008b
eb04a4b
668cc84
7efd77f
f27d5b9
f729200
9429a7b
98ce64c
7f5f477
b1b3f39
9ae0da3
a38a292
910a07b
e7326e8
a6c3880
b303ecd
0a5fda6
ccac2b0
82022e2
88e63ea
db51914
b0e99bb
008db18
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
This file was deleted.
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.
It seems odd that a component is responsible for determining if it should be displayed or not. If this is handled by the parent, then this component more or less becomes redundant.
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.
Say 1-∞ more operation modes are added.
Would you want the logic to decide which to show to be in the parent? or tucked away neatly in this component?
This component was originally build together with Anders, so it can't be all bad, right?
Since Anders and I created it, i think the only addition i have made is that while on the Text View other operation modes are not applied,
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.
Each of these modes is basically its own flow/feature. As we talked about, you would just make a component that encapsulate each flow/feature and that component would have/not have whichever components it needs to render.
This is the exact same logic for including/not including the header in different pages.
I think you would be surprised to learn how often we write bad stuff that we then go and refactor once we get smarter/learn a new detail we didn't know before. The reason you may not see this is because we dont' hesitate to refactor/delete our own code when we learn our solution is not sufficient enough.
This file was deleted.
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.
Just call it
Header
. What information doApplication
give that is not allready inHeader
?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.
@LindvedKrvang Personally I think
Header
is too generic. I would expect some sort of genericheader
element if the component is calledHeader
.Application
might not be the best fit, but then we could have a look at what kind of header this is and give it an appropriate name. Since the primary focus of the header is to display information about what media is playing and how it is played, then perhapsMediaHeader
,MediaPlayerHeader
orMediaControlHeader
? Other suggestions are welcomed 😁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.
Renamed it back to
Header
. Can't fully remember whyApplication
was added, just that it was Anders who suggested it during our talk about a number of comments.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.
After Anders late comment, i broke the Header appart such that:
1.Tthe header parts that should always be there are in a
Header
component under shared.2. A more specific version exist under
main-page
calledMediaControlHeader
, which build upon the base and adds whats needed for a header specific tomain-page
.Hope this satifies both parties.