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

InnerLoop: NavigationView build broke in recent PR #3733

Closed
Felix-Dev opened this issue Dec 1, 2020 · 5 comments · Fixed by #3742
Closed

InnerLoop: NavigationView build broke in recent PR #3733

Felix-Dev opened this issue Dec 1, 2020 · 5 comments · Fixed by #3742
Labels
area-DevInternal Internal build infrastructure, code cleanup, engineering efficiency help wanted Issue ideal for external contributors team-Controls Issue for the Controls team

Comments

@Felix-Dev
Copy link
Contributor

Felix-Dev commented Dec 1, 2020

With recent PR #3602, building the NavigationView in the inner loop now fails because some of the added test code now uses the WinUI RadioButtons control. The RadioButtons control, however, is not recognized, if it isn't part of the inner loop build process.

I see the following potential solutions here. We could either

  • include the RadioButtons control in the list of controls/elements which are included automatically in an innerloop solution build (like the SplitButton control or DropDownButton control so that the RadioButtons control is available to all future test pages. This would, however, increase the compile time for the innerloop solution across the board and building the innerloop solution is meant to be both fast and resource-friendly compared to the full solution
  • add a condition to the FeatureAreas file to build the RadioButtons control only when building the NavigationVew control or when it is explicitly listed in the InnerLoopAreas file (though the NavigationView as such is not dependent on the RadioButtons control and this would be a dependency introduced solely through the test UI)
  • modify the existing NavigationView test to use the OS XAML RadioButton control only (for example put inside a StackPanel). This would not require a lot of extra work (one can listen to the Checked/Unchecked events for each RadioButton) and would not increase the compilation time in the innerloop solution. We would als be independent of potential future test code changes (which could change the test UI elements) since the RadioButtons control is not required for the NavigationView control per-se.

@ranjeshj @chingucoding as a FYI.

@ghost ghost added the needs-triage Issue needs to be triaged by the area owners label Dec 1, 2020
@marcelwgn
Copy link
Collaborator

I think the best solution would be to reference it just for the NavigationView control, adding it to every innerloop build makes the innerloop less "inner"loop.

@Felix-Dev
Copy link
Contributor Author

Felix-Dev commented Dec 1, 2020

It is also worth pointing out that adding the RadioButtons control to the innerloop (either "globally" or only as an additional NavigationView dependency) only increases the build time the first time solution would be built. Subsequent builds will use incremental building which will not be affected by including the RadioButtons control in the build process.

Generally, I believe it makes sense to utilize WinUI controls opposed to OS XAML controls if they add benefits to the test UI. It's also likely to be a narrowly-scoped scenario (i.e. a control like NavigationView won't suddenly require a whole bunch of additional projects/controls to be built as part of its test UI). Of course, where possible, I think we should still use OS XAML to ensure the innerloop keeps efficient.

Given all this, it should be fine to include the RadioButtons control as a dependency for the NavigationView control (looking at it from an innerloop perspective) though using OS XAML also wouldn't be an issue.

@marcelwgn
Copy link
Collaborator

Yes, innerloop items would make a difference upon first compile, but if we go that route, I could also use full loop. We are only minimizing fresh compile time with the innerloop. If a WinUI control is the easier solution, I think we should use them, even if we need to adjust the innerloop because of that.

@Felix-Dev
Copy link
Contributor Author

Felix-Dev commented Dec 1, 2020

I personally try to use OS XAML where possible in my test UI because I would gladly take 1-2 more minutes development time (such as adding the different event handlers to each RadioButton) when in return I get some 15 second boost for each clean innerloop build. (I am almost exclusively using the innerloop solution so I quite often do clean innerloop builds. Other folks might not be affected by that depending on their use of innerloop vs full solution.) Of course, as I said earlier, if WinUI controls will provide "considerable" benefits in the test UI in cases for me, I would use them.

Either way, it is acceptable to me to add the RadioButtons control to the NavigationView build in the innerloop. Besides, in this case, we also benefit from both controls sharing a dependency - the ItemsRepeater control - so the additional clean build time is not increased by the full RadioButtons control build time anyway).

@ranjeshj
Copy link
Contributor

ranjeshj commented Dec 2, 2020

Adding the dependency seems like the right thing to do here. Thanks

@ranjeshj ranjeshj added help wanted Issue ideal for external contributors team-Controls Issue for the Controls team area-DevInternal Internal build infrastructure, code cleanup, engineering efficiency and removed needs-triage Issue needs to be triaged by the area owners labels Dec 2, 2020
@ghost ghost added the working on it label Dec 2, 2020
@ghost ghost removed the working on it label Dec 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-DevInternal Internal build infrastructure, code cleanup, engineering efficiency help wanted Issue ideal for external contributors team-Controls Issue for the Controls team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants