iTwin / presentation

Monorepo for iTwin.js Presentation Library
https://www.itwinjs.org/presentation/
MIT License
4 stars 0 forks source link

When rendering related instances' content as array items, include their labels next to array item index #654

Open Bordoloi123 opened 2 months ago

Bordoloi123 commented 2 months ago

For example, while displaying cant station point properties in the property pane, indexis displayed for each point and not the actual name as seen in OpenRail. image

We want the points to be displayed as image

grigasp commented 2 months ago

Hi @Bordoloi123, thanks for filing the issue. Got some questions about this:

  1. Your proposed image has the category block labeled "Cant Station Points" in plural. Ours just shows class label, which is singular. Is that a problem?

  2. We'll need to know which is property of the related class to use for display label. Is it identified in your schema somehow?

  3. Do you expect that property to be hidden from property list when you expand the instance properties list?

  4. You're proposing to show instance label instead of index when the label is not null.

    • I'm worried there could be duplicate instance labels, resulting in duplicate groups.
    • In case some related instances have labels and some don't, we could end up with a weird list where some blocks have proper labels and some have [123] ones.

    To fix the above, I suggest always showing an index and then appending the label, if there is one, e.g. [123] My label. What do you think?

grigasp commented 2 months ago

Related: https://github.com/iTwin/itwinjs-core/issues/3790

Bordoloi123 commented 2 months ago

Hi @grigasp

1 .Your proposed image has the category block labeled "Cant Station Points" in plural. Ours just shows class label, which is singular. Is that a problem?

You can ignore this. That was typo in the image.If required in plurals, we will make the changes in our schema.

2.We'll need to know which is property of the related class to use for display label. Is it identified in your schema somehow?

We have decided to set the display label through IECInstance::SetDisplayLabel , on our side. So if the display label is available, you have to just render that in property pane.

3.Do you expect that property to be hidden from property list when you expand the instance properties list? Answered in point 2.

4.You're proposing to show instance label instead of index when the label is not null. Yes

I'm worried there could be duplicate instance labels, resulting in duplicate groups. In case some related instances have labels and some don't, we could end up with a weird list where some blocks have proper labels and some have [123] ones.

This is a valid concern and it wont hamper us if the index is also displayed along the display label.

Thank you for detailed questions.

grigasp commented 2 months ago
  1. We'll need to know which is property of the related class to use for display label. Is it identified in your schema somehow?

We have decided to set the display label through IECInstance::SetDisplayLabel , on our side. So if the display label is available, you have to just render that in property pane.

  1. Do you expect that property to be hidden from property list when you expand the instance properties list?

Answered in point 2.

An ECInstance doesn't have a special label property. When you use IECInstance::SetDisplayLabel, it attempts to find a regular property that should be used as the label (using class custom attributes or property name as the means to identify it). So question 3 remains unanswered, since the property used as a label will also appear in the properties list.

Bordoloi123 commented 2 months ago
  1. We'll need to know which is property of the related class to use for display label. Is it identified in your schema somehow?

We have decided to set the display label through IECInstance::SetDisplayLabel , on our side. So if the display label is available, you have to just render that in property pane.

  1. Do you expect that property to be hidden from property list when you expand the instance properties list?

Answered in point 2.

An ECInstance doesn't have a special label property. When you use IECInstance::SetDisplayLabel, it attempts to find a regular property that should be used as the label (using class custom attributes or property name as the means to identify it). So question 3 remains unanswered, since the property used as a label will also appear in the properties list.

In that case, we will set (int the cited example) "Station" property as the display label and the property will also appear in the properties list.

grigasp commented 2 months ago

Sounds good. Here's what needs to be done:

grigasp commented 2 months ago

We should be able to implement this in itwinjs-core@4.8 timeframe.

Bordoloi123 commented 2 months ago

We should be able to implement this in itwinjs-core@4.8 timeframe.

Thank you!

grigasp commented 3 weeks ago

On hold till itwinjs-core 4.8 release. Then we'll need:

grigasp commented 2 weeks ago

Required appui changes are released with @itwin/components-react@4.16.0

grigasp commented 1 week ago

@Bordoloi123, FYI the last missing piece was just released. You should start seeing the labels after updating the following dependencies:

Please note that the label will not be visible if we can't find a label for the related instance. One of the ways to attach label property to instances of specific class is to use a instance label override rule the ruleset you use to get the properties. With it, you can say that instance label should be set to some specific property value (through property value specification).

I'll keep this issue open until you confirm that everything works as expected - please let us know.