Fix #780 : Added top/bottom for Tabs Bar in Tab Panel UI

[xantric]

Overview

1. This PR fixes or fixes part of #780
2. This PR does the following: Added tabBar_pos as a new parameter in TabUI class to specify position of the tabs bar in the Tabs Panel UI. The default value of this parameter is set as top. Currently this parameter accept top and bottom as inputs. Further PR will also add left/right inputs. This is done in order to make Tabs Panel UI more customisable.

Proof that changes work

Top

Bottom

[pep8speaks]

Hello @IshanKamboj, Thank you for updating!

Cheers! There are no style issues detected in this Pull Request. :beers: To test for issues locally, pip install flake8 and then run flake8 fury.

Comment last updated at 2024-03-25 17:48:21 UTC

[xantric]

@guaje PTAL Thank you

[ganimtron-10]

Hey @IshanKamboj, Thanks for doing this! Could you also please add the relevant tests for the changes?

[xantric]

Hey @IshanKamboj, Thanks for doing this! Could you also please add the relevant tests for the changes?

okay ... will update after adding the relevant tests

[xantric]

@ganimtron-10 PTAL .... I added the test that I think is relevant. I separated the Tabs UI test into two separate parts for top and bottom of Tabs Bar. Thank you

[guaje]

I have enabled the CI tests for this PR. Please address the respective comments and issues.

[ganimtron-10]

@ganimtron-10 PTAL .... I added the test that I think is relevant. I separated the Tabs UI test into two separate parts for top and bottom of Tabs Bar. Thank you

Thanks for the update. I will be able to look into it after Saturday meanwhile please make sure to resolve all the above comments.

[xantric]

@guaje , I combined the tests in the same function test_ui_tab_ui using a for loop, Is that acceptable? or should I do something else? ... I did this as I think that the test for the Tabs UI would remain the same and just the position of Tabs Bar will be changing and show that it still works fine. Thank you

[xantric]

@ganimtron-10 PTAL ... I am really sorry I mistakenly did a force push. Although I did the changes that you requested.

[xantric]

Hello @ganimtron-10 PTAL I changed the Tab UI test again, now I am only checking the thing affected by changing the position of Tab Bar from top to bottom. As essentially the position of the Tab Bar is changing with respect to the content panel it should only affect the elements in the content panel. So now I am just testing for the changes(addition,removal and update) in the elements for the bottom part. Also as other functionality like callbacks is not affected we need not check the same for the bottom part.

Thank you

[xantric]

Hey @ganimtron-10 just a quick reminder can you please take a look at my previous comment and changes that I made and give a feedback on if the test is okay or does it still require some other changes.

Thank you

[ganimtron-10]

Hey @IshanKamboj , Thanks for reminder. I have planned it to do by EOD.

[ganimtron-10]

Hey @IshanKamboj , I think you misunderstood about the tests. Below is a simplified explanation of how we are expecting test.

For now we have the test called test_ui_tab_ui which basically test the basic functionalities of the TabUI. As this is a new addition of feature it is better to have a separate function ie. test_ui_tab_ui_tab_position. So, this new function would have the below kind of format. You can refer this test1 or test2 for more context.

def test_ui_tab_ui_tab_position(interactive=False):
    filename = 'test_ui_tab_ui_tab_position'
    recording_filename = pjoin(DATA_DIR, filename + '.log.gz')
    expected_events_counts_filename = pjoin(DATA_DIR, filename + '.json')

    tab_ui_top = ui.TabUI(
        position=(50, 50), size=(300, 300), nb_tabs=3, draggable=True,
        tab_bar_pos='top')

    tab_ui_bottom = ui.TabUI(
        position=(50, 50), size=(300, 300), nb_tabs=3, draggable=True,
        tab_bar_pos='bottom'
    )

    # test and assert various properties, positions and changes which is affected by the change

    # Create Event counter for both tab_ui_top and tab_ui_bottom

    # Create a single scene for both the tab
    # Add both the tabs to the scene

    if interactive:
        # manage and record events for both tab ui's
        # while you are recording test try collapsing various tabs, changing tabs and moving them around for both tab ui
    else:
        # replay events for both tab ui's

    # check the collapse, and changes count

[xantric]

Hey @IshanKamboj , I think you misunderstood about the tests. Below is a simplified explanation of how we are expecting test.

For now we have the test called test_ui_tab_ui which basically test the basic functionalities of the TabUI. As this is a new addition of feature it is better to have a separate function ie. test_ui_tab_ui_tab_position. So, this new function would have the below kind of format. You can refer this test1 or test2 for more context.

def test_ui_tab_ui_tab_position(interactive=False):
    filename = 'test_ui_tab_ui_tab_position'
    recording_filename = pjoin(DATA_DIR, filename + '.log.gz')
    expected_events_counts_filename = pjoin(DATA_DIR, filename + '.json')

    tab_ui_top = ui.TabUI(
        position=(50, 50), size=(300, 300), nb_tabs=3, draggable=True,
        tab_bar_pos='top')

    tab_ui_bottom = ui.TabUI(
        position=(50, 50), size=(300, 300), nb_tabs=3, draggable=True,
        tab_bar_pos='bottom'
    )

    # test and assert various properties, positions and changes which is affected by the change

    # Create Event counter for both tab_ui_top and tab_ui_bottom

    # Create a single scene for both the tab
    # Add both the tabs to the scene

    if interactive:
        # manage and record events for both tab ui's
        # while you are recording test try collapsing various tabs, changing tabs and moving them around for both tab ui
    else:
        # replay events for both tab ui's

    # check the collapse, and changes count

Understood now 👍 , will do it this way. Sorry for the confusion above Thank you

[xantric]

@ganimtron-10 PTAL now, I added the test in the way you explained. Please tell if there are still any issues with it. Thank you

[xantric]

Hey @ganimtron-10 PTAL I added the test in the way you asked. If there is still some issues please let me know. Thank you

[ganimtron-10]

Hey @IshanKamboj , This looks good to me. @skoudoro @guaje PTAL.

[skoudoro]

I will look into it this week. Thank you the review @ganimtron-10 and thank you for the work @IshanKamboj

[skoudoro]

Also, can you update viz_tab tutorial to show this features @IshanKamboj. thank you

[xantric]

Hey @skoudoro PTAL, I updated the viz_tab tutorial to include this feature. Sorry for the delay Thank you

[xantric]

Hey @skoudoro just a quick reminder PTAL. Thank you

[skoudoro]

@maharshi-gor, Can you try and look at this? I do not find the time to do a deeper test and look in details.

Thank you in advance.

@IshanKamboj, sorry for the delay

[xantric]

@skoudoro PTAL, I added the warning. Should I use logging or is this ok? ... Sorry for the late reply Thank you

[xantric]

@skoudoro I added the warning as u specified..... also I only need to check self.tab_bar_pos is top/bottom at the beginning of the function and if it is not then I just set it top by default so both the conditions below that should work.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 61.53846% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 84.44%. Comparing base (ea0fb16) to head (3d99a0e). Report is 2 commits behind head on master.

Additional details and impacted files

[](https://app.codecov.io/gh/fury-gl/fury/pull/855?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=fury-gl) ```diff @@ Coverage Diff @@ ## master #855 +/- ## ========================================== + Coverage 84.42% 84.44% +0.01% ========================================== Files 44 44 Lines 10529 10540 +11 Branches 1418 1423 +5 ========================================== + Hits 8889 8900 +11 + Misses 1268 1266 -2 - Partials 372 374 +2 ``` | [Files](https://app.codecov.io/gh/fury-gl/fury/pull/855?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=fury-gl) | Coverage Δ | | |---|---|---| | [fury/ui/containers.py](https://app.codecov.io/gh/fury-gl/fury/pull/855?src=pr&el=tree&filepath=fury%2Fui%2Fcontainers.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=fury-gl#diff-ZnVyeS91aS9jb250YWluZXJzLnB5) | `83.73% <61.53%> (-0.66%)` | :arrow_down: | ... and [1 file with indirect coverage changes](https://app.codecov.io/gh/fury-gl/fury/pull/855/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=fury-gl)

[xantric]

@skoudoro just a quick reminder please review the changes I made... Thank you

[skoudoro]

Waiting for the CI's to run and then I will go ahead and merge this PR. It seems ok. Thank you for this @IshanKamboj.

[skoudoro]

Thank you for this work @IshanKamboj !