-
-
Notifications
You must be signed in to change notification settings - Fork 531
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
DataGrid: Expose public info about DetailRow being expanded #5949
base: master
Are you sure you want to change the base?
DataGrid: Expose public info about DetailRow being expanded #5949
Conversation
…ataGrid-add-API-that-tells-us-if-detail-row-is-expanded-or-present
I think something is not right with the PR. Too many files is changed. Have you used the wrong target branch? |
That's because I merged the 1.7 to this branch, because I needed the code example. If you merge 1.7 to master the extra code won't be part of this pr anymore. |
…il-row-is-expanded-or-present
@@ -414,7 +414,7 @@ public bool RemoveColumn( DataGridColumn<TItem> column ) | |||
/// Links the child row with this datagrid. | |||
/// </summary> | |||
/// <param name="row">Row to remove.</param> | |||
public bool RemoveRow( DataGridRowInfo<TItem> row ) | |||
public bool RemoveRow( _DataGridRowInfo<TItem> row ) |
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 think it makes sense to mix public API and private APIs. In this case we have public methods that are working with our "private" models.
I would leave it as previously, as DataGridRowInfo
. And then make some fields as internal. If possible.
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.
This change comes form the rename of DataGridRowInfo
to _DataGridRowInfo
.
There is no change in functionality or accessibility or class usage
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.
There is no change in functionality or accessibility or class usage
True, but it is still a breaking change. And now we have a private named class publicly exposed.
Revert to the original name, as we don't want this kind if naming for our API.
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.
Ok, I reverted back and renamed created the DataGridRowPublicInfo
...
I still don't quite understand the conventions here and the public/private/internal logic behind this.
Why are some members marked public
, while used only for internal stuff? Why are some of those named with _
prefix and some are not?
Description
Closes #5947
This update exposes
DataGridRowInfo.DetailRowExpanded
, requiring several adjustments:Renamed the original
DataGridRowInfo
:Changed to
_DataGridRowInfo
to reflect its private nature while retainingDataGridRowInfo
as the publicly exposed name. Alternatives considered includeRowInfo
,DataGridRowInfoPublic
, etc.Introduced a new public
DataGridRowInfo
:This class now directly exposes
DetailRowExpanded
. And can be used in a future to expose more a different info.Renamed
HasDetailRow
toDetailRowExpanded
:As mentioned in the issue description. An alternative like
DetailRowVisible
is possible, given the existingDetailRowStartsVisible
, butExpanded
feels better. It might also be worth renamingDetailRowStartsVisible
toDetailRowStartsExpanded
for consistency..Renamed
GetRowInfo
toGetRowInfoPrivate
:This change allows
GetRowInfo
to be used as the public method.Merged 1.7, as it required a sample and a functional gh action for testing.