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.

Sign up for GitHub

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

Jump to bottom

Option to put TabContainer tabs at bottom #82468

Merged
merged 1 commit into from
Jan 8, 2024
Merged

Option to put TabContainer tabs at bottom #82468

merged 1 commit into from
Jan 8, 2024

Conversation

kitbdev
Copy link
Contributor

@kitbdev kitbdev commented Sep 28, 2023

part of: godotengine/godot-proposals#1986
salvaged: #44420

Adds tabs_at_bottom to TabContainer.
Enabling will move the tab bar to the bottom.

image

@kitbdev kitbdev requested review from a team as code owners September 28, 2023 03:02
@Calinou Calinou added this to the 4.x milestone Sep 28, 2023
YeldhamDev
YeldhamDev requested changes Sep 28, 2023
View reviewed changes
Copy link
Member

@YeldhamDev YeldhamDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be a specific set of theme styles for the upside-down tabs.

@Maran23
Copy link
Contributor

Maran23 commented Sep 28, 2023

and especially for the future, this should be an enum. Right now it would only contain TOP and BOTTOM. TOP would be the default and this way we do not break compatibility (also when adding the last two directions LEFT and RIGHT at one point)

@MewPurPur
Copy link
Contributor

MewPurPur commented Sep 28, 2023 •
edited
Loading

Related: #63873

@kitbdev kitbdev force-pushed the tabcontainer-bottom branch 2 times, most recently from 4efc8a2 to a215a97 Compare September 28, 2023 16:09
@kitbdev kitbdev requested a review from a team as a code owner September 28, 2023 16:09
@kitbdev kitbdev force-pushed the tabcontainer-bottom branch from a215a97 to 87e4ec9 Compare September 28, 2023 16:12
@kitbdev
Copy link
Contributor Author

kitbdev commented Sep 28, 2023

Changed to use an enum TabPosition.
I used a theme variation for the bottom style TabBarBottom that is set when the position is set. TabContainer now sets its internal TabBar to use the same theme variation.
Editor theme variation (for testing):
image
Default theme variation:
image

@kitbdev kitbdev force-pushed the tabcontainer-bottom branch 2 times, most recently from df15d2e to 84869ff Compare September 28, 2023 17:30
@kitbdev
Copy link
Contributor Author

kitbdev commented Sep 28, 2023

fixed docs and don't override type variation if set

@kitbdev kitbdev force-pushed the tabcontainer-bottom branch 2 times, most recently from c0218fc to 59ecaff Compare September 28, 2023 23:47
@kitbdev
Copy link
Contributor Author

kitbdev commented Sep 29, 2023

changed to use theme item for _bottom styles in tab container. I found a way to not have to modify TabBar at all using the theme overrides! That was a big concern I had with this method originally.

I am now looking into flipping the StyleBox automatically

@kitbdev kitbdev force-pushed the tabcontainer-bottom branch from 59ecaff to 5e919ec Compare September 29, 2023 02:54
@kitbdev
Copy link
Contributor Author

kitbdev commented Sep 29, 2023

Now automatically flips the tab's stylebox instead of using separate theme items.

@kitbdev kitbdev force-pushed the tabcontainer-bottom branch 2 times, most recently from 7daba2d to 8239817 Compare September 29, 2023 03:00
YeldhamDev
YeldhamDev approved these changes Sep 29, 2023
View reviewed changes
Copy link
Member

@YeldhamDev YeldhamDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kitbdev kitbdev mentioned this pull request Oct 22, 2023
Closed
@kitbdev kitbdev force-pushed the tabcontainer-bottom branch from 8239817 to 68deb34 Compare November 9, 2023 23:45
@kitbdev
Copy link
Contributor Author

kitbdev commented Nov 9, 2023

Rebased and fixed merge conflicts.
Removed unnecessary line: tab_bar->set_theme_type_variation(get_theme_type_variation());

@akien-mga akien-mga requested a review from KoBeWi January 4, 2024 14:12
KoBeWi
KoBeWi reviewed Jan 6, 2024
View reviewed changes
scene/gui/tab_bar.h Outdated Show resolved Hide resolved
@kitbdev kitbdev force-pushed the tabcontainer-bottom branch from 68deb34 to 90f216f Compare January 6, 2024 16:57
KoBeWi
KoBeWi reviewed Jan 6, 2024
View reviewed changes
scene/gui/tab_container.cpp Outdated
@@ -198,6 +220,8 @@ void TabContainer::_on_theme_changed() {
return;
}

tab_bar->set_tab_style_v_flip(tabs_position == POSITION_BOTTOM);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this here if it doesn't depend on the theme?

@KoBeWi
Copy link
Member

KoBeWi commented Jan 6, 2024 •
edited
Loading

There is a small difference between popup position in top and bottom tabs:
image
image
I think the bottom one looks better and the top one looks wrongly implemented (it uses icon height, not the actual header size).

KoBeWi
KoBeWi reviewed Jan 6, 2024
View reviewed changes
scene/gui/tab_container.cpp Outdated Show resolved Hide resolved
KoBeWi
KoBeWi reviewed Jan 6, 2024
View reviewed changes
scene/gui/tab_container.cpp Outdated Show resolved Hide resolved
AThousandShips
AThousandShips reviewed Jan 6, 2024
View reviewed changes
scene/gui/tab_bar.cpp Outdated Show resolved Hide resolved
scene/gui/tab_bar.cpp Outdated Show resolved Hide resolved
@kitbdev kitbdev force-pushed the tabcontainer-bottom branch from 90f216f to f6a2128 Compare January 6, 2024 20:31
@kitbdev
Copy link
Contributor Author

kitbdev commented Jan 6, 2024

Resolved issues.
I made the icon popup appear at the bottom or top of the menu icon. I don't just use the tab height in case it is large, the popup would look disconnected.
image
image

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Jan 8, 2024
@akien-mga akien-mga requested a review from KoBeWi January 8, 2024 07:58
@akien-mga akien-mga merged commit 129332e into godotengine:master Jan 8, 2024
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@kitbdev kitbdev deleted the tabcontainer-bottom branch January 8, 2024 17:36
@nathanfranke nathanfranke mentioned this pull request Sep 18, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AThousandShips AThousandShips AThousandShips approved these changes

@YeldhamDev YeldhamDev YeldhamDev approved these changes

@KoBeWi KoBeWi KoBeWi approved these changes

Assignees
No one assigned
Labels
enhancement topic:gui
Projects
None yet
Milestone
4.3
Development

Successfully merging this pull request may close these issues.
8 participants
@kitbdev @Maran23 @MewPurPur @KoBeWi @akien-mga @YeldhamDev @AThousandShips @Calinou