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

Tabs: Improve scrolling #3556

Merged
merged 9 commits into from
Mar 19, 2022
Merged

Tabs: Improve scrolling #3556

merged 9 commits into from
Mar 19, 2022

Conversation

Flaflo
Copy link
Contributor

@Flaflo Flaflo commented Dec 15, 2021

Description

Fixes #3473, Fixes #3012
Also this fixes a bug with center scrolling on re-render and improves tabs inside dialogs
Scrolling now ensures, that the full bounds of the last tab is visible before disabling scrolling buttons
Scrolling amount is now dependent on the amount of fully visible tab panels that fit into the tab area

How Has This Been Tested?

visually

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Before
beforeBig
beforeSmall

After
afterBig
afterSmall

Checklist:

  • The PR is submitted to the correct branch (dev).
  • My code follows the code style of this project.
  • I've added relevant tests.

@Flaflo
Copy link
Contributor Author

Flaflo commented Dec 15, 2021

Can please someone assist fixing the tests?

@Garderoben Garderoben added the bug Something does not work as intended/expected label Dec 15, 2021
@just-the-benno
Copy link
Contributor

@Flaflo

I've you allow me to push into your branch, I can help you fixing the tests

@Flaflo
Copy link
Contributor Author

Flaflo commented Dec 15, 2021

@Flaflo

I've you allow me to push into your branch, I can help you fixing the tests

Sent you an invite to the fork

@mikes-gh
Copy link
Contributor

mikes-gh commented Dec 15, 2021

I noticed a change to single tab scrolling.
Definitely the tabs should show as whole tabs (which this PR fixes thankyou) but the original design was more like a paging of tabs which gets you to your new tab quicker. But maybe this PR's behaviour (one at a time) is what people want/expect or maybe we can have both behaviours?

@Flaflo
Copy link
Contributor Author

Flaflo commented Dec 16, 2021

I noticed a change to single tab scrolling. Definitely the tabs should show as whole tabs (which this PR fixes thankyou) but the original design was more like a paging of tabs which gets you to your new tab quicker. But maybe this PR's behaviour (one at a time) is what people want/expect or maybe we can have both behaviours?

Bugfree pagination would require a complete rewrite of the scrolling and personally I am not a fan of it especially on mobile.
Its way easier for the eyes if you go one tab at a time. Vuetify's tab scrolling suffers from similiar bugs mudblazor does at the moment.
We could implement the old scrolling as some kind of second scrolling mode but not as the default one imho.

@henon
Copy link
Collaborator

henon commented Dec 16, 2021

I like the new scrolling.

@mikes-gh mikes-gh changed the title Improving MudTabs scrolling Tabs: Improve scrolling Dec 16, 2021
@Garderoben
Copy link
Member

Its designed like that on purpose, this is not a improvement nor should replace current behavior.
If it doesn't add to much complexity we could have it as a secondary, optional scrolling behavior if its possible. Not very nice to have to click 5 times to see the last item, very annoying.

The tab scrolling is now paginated.
Scrolling amount depends on the amount of completly visible panels inside the tab content area.
@Flaflo
Copy link
Contributor Author

Flaflo commented Dec 20, 2021

@just-the-benno I significantly changed the scrolling behaviour FYI

@Flaflo
Copy link
Contributor Author

Flaflo commented Dec 20, 2021

I noticed a change to single tab scrolling. Definitely the tabs should show as whole tabs (which this PR fixes thankyou) but the original design was more like a paging of tabs which gets you to your new tab quicker. But maybe this PR's behaviour (one at a time) is what people want/expect or maybe we can have both behaviours?

@mikes-gh I changed scrolling behavior again, it now uses a more accurate pagination.

@Garderoben
Copy link
Member

I like this one, this feels like a improvement and something in-between your first and the current one we have, unless the API change this would not really be a breaking change either? (eg the behavior)

@just-the-benno i leave you to the code review or some one else if you don't have time, but it has my "approval"

@henon
Copy link
Collaborator

henon commented Dec 20, 2021

Code looks good

@Flaflo
Copy link
Contributor Author

Flaflo commented Dec 20, 2021

I like this one, this feels like a improvement and something in-between your first and the current one we have, unless the API change this would not really be a breaking change either? (eg the behavior)

@just-the-benno i leave you to the code review or some one else if you don't have time, but it has my "approval"

The API has not changed. It is technically not a breaking change and is not too far off of the current behavior so this should be fine.

@mikes-gh
Copy link
Contributor

mikes-gh commented Dec 22, 2021

Obviously tests need investigating.
I noticed for example when clicking 3 in the docs samples the tabs scroll.
To me that unexpected.

Kapture 2021-12-22 at 20 58 38

@Flaflo
Copy link
Contributor Author

Flaflo commented Jan 3, 2022

The scrolling to center when clicking tabs has nothing todo with my pullrequest. This behavior was like this before. Also Vuteify hast the same behavior.

Or am I wrong?

@Flaflo
Copy link
Contributor Author

Flaflo commented Jan 13, 2022

@just-the-benno Any news from your side?

@MattChique
Copy link
Contributor

Any news?

@mikes-gh
Copy link
Contributor

Hi @MattChique

Just the tests failures need understanding/fixing. Do you have time to look at it?

@mikes-gh
Copy link
Contributor

@Flaflo Would you mind allowing pushes to this PR please in case someone else wants to fix the tests? 🙏

@Flaflo
Copy link
Contributor Author

Flaflo commented Feb 1, 2022

@Flaflo Would you mind allowing pushes to this PR please in case someone else wants to fix the tests? 🙏

How do I do that?

@henon
Copy link
Collaborator

henon commented Feb 1, 2022

Reopen a new PR and check the respective checkbox in the process.

@mikes-gh
Copy link
Contributor

mikes-gh commented Feb 1, 2022

Do you have allow edits by maintainers ticked (see bottom right)

image

@Flaflo
Copy link
Contributor Author

Flaflo commented Feb 1, 2022

Do you have allow edits by maintainers ticked (see bottom right)

image

Yes, it was enabled all the time

@alexhiggins732
Copy link

Bump on this PR. What is left to complete?

@Flaflo
Copy link
Contributor Author

Flaflo commented Feb 11, 2022

Bump on this PR. What is left to complete?

Unit-Tests need to be rewritten

@codecov
Copy link

codecov bot commented Mar 19, 2022

Codecov Report

Merging #3556 (c51ed24) into dev (9e4be88) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              dev    #3556      +/-   ##
==========================================
+ Coverage   91.18%   91.19%   +0.01%     
==========================================
  Files         359      359              
  Lines       12394    12402       +8     
==========================================
+ Hits        11301    11310       +9     
+ Misses       1093     1092       -1     
Impacted Files Coverage Δ
src/MudBlazor/Components/Tabs/MudTabs.razor.cs 98.03% <100.00%> (+0.46%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9e4be88...c51ed24. Read the comment docs.

@henon
Copy link
Collaborator

henon commented Mar 19, 2022

@Flaflo shame on you for leaving a good PR to rot for months. See how long it took me to fix them three tests, 7 minutes! ;)

image

Nevertheless, big thanks for perfecting the tab scrolling behavior @Flaflo.

@henon henon merged commit 975f7d4 into MudBlazor:dev Mar 19, 2022
@henon henon added enhancement New feature or request and removed needs tests labels Mar 19, 2022
@henon henon added this to the 6.0.8 milestone Mar 19, 2022
@alexhiggins732
Copy link

Wow... It just a single community member calling out this bug to get it merged. This bug was out there for months... across many releases and many people saw it and ignored it. Good grief.

jammerware pushed a commit to jammerware/MudBlazor that referenced this pull request Sep 20, 2022
* Improved Tabs (Fixes MudBlazor#3473, Fixes MudBlazor#3012)

Co-authored-by: Meinrad Recheis <meinrad.recheis@gmail.com>
jammerware pushed a commit to jammerware/MudBlazor that referenced this pull request Sep 20, 2022
* Improved Tabs (Fixes MudBlazor#3473, Fixes MudBlazor#3012)

Co-authored-by: Meinrad Recheis <meinrad.recheis@gmail.com>
jammerware pushed a commit to jammerware/MudBlazor that referenced this pull request Sep 20, 2022
* Improved Tabs (Fixes MudBlazor#3473, Fixes MudBlazor#3012)

Co-authored-by: Meinrad Recheis <meinrad.recheis@gmail.com>
jammerware pushed a commit to jammerware/MudBlazor that referenced this pull request Sep 20, 2022
* Improved Tabs (Fixes MudBlazor#3473, Fixes MudBlazor#3012)

Co-authored-by: Meinrad Recheis <meinrad.recheis@gmail.com>
3dots pushed a commit to 3dots/MudBlazor that referenced this pull request Mar 23, 2023
* Improved Tabs (Fixes MudBlazor#3473, Fixes MudBlazor#3012)

Co-authored-by: Meinrad Recheis <meinrad.recheis@gmail.com>
ferraridavide pushed a commit to ferraridavide/MudBlazor that referenced this pull request May 30, 2023
* Improved Tabs (Fixes MudBlazor#3473, Fixes MudBlazor#3012)

Co-authored-by: Meinrad Recheis <meinrad.recheis@gmail.com>
ilovepilav pushed a commit to ilovepilav/MudBlazor that referenced this pull request Nov 25, 2023
* Improved Tabs (Fixes MudBlazor#3473, Fixes MudBlazor#3012)

Co-authored-by: Meinrad Recheis <meinrad.recheis@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something does not work as intended/expected enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MudTabs tab text cut off on mobile MudTabs in MudDialog: wrong scroll and unable to select
7 participants