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
812 landing popup #934
812 landing popup #934
Changes from 33 commits
2e06eaf
62b5f5b
713b37f
18834f4
02b83fd
e920f50
3dcccf3
5d33893
f973beb
ebe79c1
a04ee4a
34ce78b
aa253fa
6666237
eb3bffa
0bd57c6
b86ccff
4f01d31
5d18417
843a590
a0e2ce2
5b34678
5448206
ec595bc
b03ddc1
6fd0b41
bdc373b
464c42c
11955bc
881d88f
69d5c71
f342db9
3604eb0
daa14c6
cce8655
ed6bafc
98b2e0a
a629e3c
f0bbf52
1ea1950
1e1fa37
f26ee16
c47552e
7bebae8
93eee85
0fa96e6
c3e928b
deed4d8
4d54613
affd16f
3109ee2
7743f6e
530d1f5
26c82f6
ad0ad10
55e7919
83878e8
2b4ae97
c92e925
684ef3f
3da289f
d9c7472
3cacf36
ff1cf3b
95397de
e6049a0
7d75399
1dffcfd
8f111e8
4eb1a39
c6a0203
7b43211
0ad23da
f0f60aa
1d21305
39975e6
16af597
e95511a
0179152
9e700ab
bc25503
09c7ab7
ab29aab
064e382
64f7215
91286e3
132872b
2d25bd9
00670d7
4ceb8eb
30fb85d
4f651c4
0200dc2
87af18a
2434c2b
22e057e
71c1676
9de95b1
970ac66
8825276
989f30d
0670e3b
007c0e1
0b4ddf8
9e7ecfb
e847ac9
10ac427
3300979
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Moving reporter to last because it's used internally (I think).
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.
@chlebowa what do you mean by "used internally"?
reporter_previewer_module
is an exported module in teal.@m7pr please remember to change a constructor call in
reporter_previewer_module
to use atype
argument instead of thisExistence of
reporter_previewer_module
provoke me to think where these "special" modules should be placed. Inteal
orteal.modules.general
?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 mean an app dev does not create one by hand but one is created by
teal
if any of the modules has a reporter.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.
Yes
reporter_previewer_module
is added automatically if anyteal_module
has reporter argument. But also,reporter_previewer_module
can be included by app developer in the list of modules. App developer have ability to change previewer within exposed argumentslabel
andserver_args
.edit: So, from my point of view landing and previewer are special types of modules, which are managed in a special way by teal. Both can be added by app developer and changed appereance by changing its server arguments. If both are in
teal
thentype
argument doesn't make sense - it could handled in the same way we do this currently with reporter_previewer (class(module) <- "teal_module_reporter"
).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.
In that case feel free to ignore the suggestion. But it wouldn't hurt to arrange the classes in the order of descending use frequency.
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 having the two special modules being created with
@pawelru disagrees
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.
so we do assign a class for
tm_landing_page
the same way we assign class forreporter_previewer_module
?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.
Yes but I believe part of this opinion was driven by hidden reversed dependency between setting class in tmg and internals of teal. In suggestion everything stays in
teal
(internals) and relevant comment should give us developers sufficient hint what is going on.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 moved
tm_landing_popup
toteal
and removedtype
argument frommodule
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.
Yes, I don't think Pawel's point is valid anymore
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.