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

OSOE-47: Open-source Data Tables #80

Merged
merged 123 commits into from
Feb 21, 2022
Merged

OSOE-47: Open-source Data Tables #80

merged 123 commits into from
Feb 21, 2022

Conversation

sarahelsaig
Copy link
Member

@sarahelsaig sarahelsaig commented Jan 19, 2022

CHANGE BASE BRANCH TO DEV BEFORE MERGE.

OSOE-47

This merges the release upgrade done in FINI-859. It might be worthwhile to review that PR first, but do as you please. :) This branch further upgrades from Orchard Core 1.0.0 to 1.2.1 but there were no issues with that.

# Conflicts:
#	ResourceManagementOptionsConfiguration.cs
# Conflicts:
#	ResourceManagementOptionsConfiguration.cs
(cherry picked from commit 6beca87)
(cherry picked from commit 0c56c67)
Comment on lines 13 to 15
@* Forces the CultureInfo to use the canonical date format. This prevents user or system level customizations. We
need this for the UI tests to work reliably. *@
var cultureInfo = new CultureInfo(CultureInfo.CurrentCulture.Name, useUserOverride: false);
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't have the foggiest why this came up only now, so any insight is appreciated. I think it might be related to when you edit Time & Language > _Region> Change data format in the system Settings app, but I'm not super sure.

Copy link
Member

Choose a reason for hiding this comment

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

Isn't useUserOverride something that you can specify in the format string? Because then we can use the DateTime shape. If not, shouldn't this be added to the shape as well? Please open an OC issue.

Copy link
Member Author

@sarahelsaig sarahelsaig Feb 20, 2022

Choose a reason for hiding this comment

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

Isn't useUserOverride something that you can specify in the format string?

No, you can only set IFormatProvider which defaults to CultureInfo.CurrentCulture which apparently can be affected by your system settings.

Because then we can use the DateTime shape. If not, shouldn't this be added to the shape as well? Please open an OC issue.

Does the shape even accept a format provider? I can't find a full documentation for that shape and it's tough to search for it in the OC source code becuase "DateTime" is kind of a common word.

Copy link
Member

Choose a reason for hiding this comment

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

It doesn't except a format provider but only a format string, see here.

Copy link
Member Author

@sarahelsaig sarahelsaig Feb 21, 2022

Choose a reason for hiding this comment

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

Thanks. I think what they do is very reasonable, since OC is supposed to to set the current UI culture. Let me check if I can find something bug reportable on that end.

Copy link
Member Author

Choose a reason for hiding this comment

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

Generally I think the problem is with how OC sets the CurrentCulture/CurrentUICulture. I have made a bug report with a suggested solution here. I also have added this to the comment above.

Copy link
Member Author

Choose a reason for hiding this comment

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

They don't seem to be super receptive to it. However I have a fallback issue here.
Also I think it's a valid point that the server shouldn't customize the date format in the first place, so why is it in CI?

Copy link
Member

Choose a reason for hiding this comment

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

Replied there.

Comment on lines 13 to 15
@* Forces the CultureInfo to use the canonical date format. This prevents user or system level customizations. We
need this for the UI tests to work reliably. *@
var cultureInfo = new CultureInfo(CultureInfo.CurrentCulture.Name, useUserOverride: false);
Copy link
Member

Choose a reason for hiding this comment

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

Isn't useUserOverride something that you can specify in the format string? Because then we can use the DateTime shape. If not, shouldn't this be added to the shape as well? Please open an OC issue.

Lombiq.DataTables/Lombiq.DataTables.csproj Show resolved Hide resolved
Copy link
Member

@Piedone Piedone left a comment

Choose a reason for hiding this comment

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

See comments.

Readme.md Outdated Show resolved Hide resolved
Co-authored-by: Zoltán Lehóczky <[email protected]>
Lombiq.DataTables.Samples/Lombiq.DataTables.Samples.csproj Outdated Show resolved Hide resolved
Lombiq.DataTables.Samples/Readme.md Outdated Show resolved Hide resolved
Lombiq.DataTables.Samples/Readme.md Outdated Show resolved Hide resolved
Lombiq.DataTables.Samples/Readme.md Outdated Show resolved Hide resolved
Comment on lines 13 to 15
@* Forces the CultureInfo to use the canonical date format. This prevents user or system level customizations. We
need this for the UI tests to work reliably. *@
var cultureInfo = new CultureInfo(CultureInfo.CurrentCulture.Name, useUserOverride: false);
Copy link
Member

Choose a reason for hiding this comment

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

Replied there.

Lombiq.DataTables.Samples/Readme.md Outdated Show resolved Hide resolved
@Piedone Piedone changed the base branch from issue/OSOE-47-base to dev February 21, 2022 19:25
@Piedone Piedone merged commit 21753ac into dev Feb 21, 2022
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