Added ROI Set name to the ROI menu

[milandomazet]

Hello, this is a small PR related to the ROI Set name being displayed in the ROI menu. It seemed like it might be valuable displaying it since it is already in the table and it can be defined by the user when providing ROIs through config.

Just ROI Set defined

Both ROI Set and Description/Name defined

Only Description/Name defined

[jrobinso]

@milandomazet @turner Thanks for the PR. I agree with the intent but I don't think we need to add the 'roiSet' to every feature feature object in the set. Could we try just passing the roiSet itself as a parameter to the ROIMenu.present function?

            this.roiMenu.present(feature, isUserDefined, event, this, columnContainer, regionElement)

The paremeter "isUserDefined" is itself just a property of ROISet, so it can be replaced as we are now using 2 properties

this.roiMenu.present(feature, roiSet, event, this, columnContainer, regionElement)

This can in turn get passed on to menuItems in ROIMenu and used to get the roiSet name (and "isUserDefined")

    menuItems(feature, roiSet, event, roiManager, columnContainer, regionElement) {

[milandomazet]

Thanks a lot on your feedback @jrobinso it makes a lot of sense to do it that way. I just added another commit with that approach.

[turner]

@milandomazet can you test with long strings? We might need to tweak the css.

[milandomazet]

@turner long strings don't get wrapped I've noticed that. Maybe I could add a div and styles to it?

[turner]

@turner long strings don't get wrapped I've noticed that. Maybe I could add a div and styles to it?

Take a look at _roi.scss where I define all ROI related styling. All you should need to do is add text-overflow: ellipsis; for child divs of the parent to the divs you are adding.

[milandomazet]

@turner the problem is that the styling in ROIMenu is not used at all as the menuPopup class is being called to display the popup this.browser.menuPopup.presentTrackContextMenu(event, menuItems) we would have to change that class, enable passing of the container somehow or duplicate its functions in ROIMenu to change this specific popup.

On the other hand we could change the menuPopup styling and that is what I've done in the last commit. Currently the text is wrapped (to be honest it looks better to me this way) but we can do ellipsis as well, both screenshots are attached.

If we decide to go this route maybe we should delete container part code of the ROIMenu as well.

[turner]

Can you in addition add the ellipsis? Thx.

[turner]

Can you in addition add the ellipsis? Thx.

Actually, we can forget the ellipsis. I thinkt this is fine.

[milandomazet]

Thanks @turner! Should I remove the component creation part of the ROIMenu as it is not used at the moment from this class or should I leave it for any future work on it? The code in question is in the constructor:

    // container
    this.container = DOMUtils.div({class: 'igv-roi-menu'})
    parent.appendChild(this.container)

    // header
    const header = DOMUtils.div()
    this.container.appendChild(header)

    UIUtils.attachDialogCloseHandlerWithParent(header, () => this.container.style.display = 'none')

    // body
    this.body = DOMUtils.div()
    this.container.appendChild(this.body)

    this.container.style.display = 'none'

[turner]

Thanks @turner! Should I remove the component creation part of the ROIMenu as it is not used at the moment from this class or should I leave it for any future work on it? The code in question is in the constructor:

    // container
    this.container = DOMUtils.div({class: 'igv-roi-menu'})
    parent.appendChild(this.container)

    // header
    const header = DOMUtils.div()
    this.container.appendChild(header)

    UIUtils.attachDialogCloseHandlerWithParent(header, () => this.container.style.display = 'none')

    // body
    this.body = DOMUtils.div()
    this.container.appendChild(this.body)

    this.container.style.display = 'none'

Leave this as is for now. That is part of a refactor we will get to at some point in the future.