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

feat(DUI2): Cnx 9068 update dui2 to use new fe2 terminology #3200

Merged
merged 7 commits into from
Feb 28, 2024

Conversation

clairekuang
Copy link
Member

@clairekuang clairekuang commented Feb 27, 2024

Description & motivation

A button that serves as a global config toggle for fe2 terminology was previously implemented in the home view of DUI2, and used in conjunction with fe2valueconverter to toggle terminology in the home view. In the stream view, the account was used to determine the value of UseFe2 irrespective of the config value.

This pr:

  • removes functionality for converting fe2 terminology in both views and viewmodels
  • All user-facing strings have been updated to reflect new fe2 terminology

Copy link
Contributor

@AlanRynne AlanRynne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. There are some kinks I'm willing to accept since this is DUI2 we're talking about

{
return JsonConvert.DeserializeObject<Config>(newConfig);
deserializedConfig.UseFe2 = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I correct in saying that UseFe2 will always be true after this.
No more FE1 terminology.

If that's the case, would we be able to kill the Fe2TextValueConverter and Formatting.ReplaceTerminology

Maybe not now, but we may want to do this eventually. Are there any advantages to keep it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update to the decision: we'll open a separate ticket to deprecate terminology-switching code

Copy link
Member Author

@clairekuang clairekuang Feb 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update no. 2: I've decided to remove everything because it was about the same amount of work amending the previous pr anyways

@clairekuang clairekuang marked this pull request as draft February 28, 2024 12:59
@clairekuang clairekuang marked this pull request as ready for review February 28, 2024 13:30
@@ -535,7 +535,7 @@ private void GenerateMenuItems()
new MenuItemViewModel(LaunchManagerCommand, "Manage accounts in Manager", MaterialIconKind.AccountCog)
);

menu.Items.Add(new MenuItemViewModel(RefreshCommand, "Refresh streams & accounts", MaterialIconKind.Refresh));
menu.Items.Add(new MenuItemViewModel(RefreshCommand, $"Refresh projects & accounts", MaterialIconKind.Refresh));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few of these strings have an unnecessary $
Perhaps we can quickly remove them before his pr is merged

@@ -199,7 +195,7 @@
<StackPanel>
<TextBlock
IsVisible="{Binding !Commits.Count}"
Text="{Binding UseFe2, Converter={StaticResource Fe2TextValueConverter}, ConverterParameter='This branch has no commits. Commits are generated what data is sent to a Branch in a Stream.&#x0a;&#x0a;Please select a different Branch or Stream.'}"
Text="This model has no version. Versions are generated what data is sent to a model in a project.&#x0a;&#x0a;Please select a different model or project."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change in pluralisation. Was this intended?
"This branch has no commits"
"This model has no version"

(I think either is fine, just want to make sure this change was intentional

@JR-Morgan JR-Morgan self-requested a review February 28, 2024 13:59
@JR-Morgan JR-Morgan merged commit 8f9ea2d into dev Feb 28, 2024
4 checks passed
@JR-Morgan JR-Morgan deleted the CNX-9068-DUI2-Update-to-use-new-FE2-Terminology branch February 28, 2024 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants