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

Toolbox-custom-order #14759

Merged
merged 1 commit into from
May 23, 2024
Merged

Toolbox-custom-order #14759

merged 1 commit into from
May 23, 2024

Conversation

hristoterezov
Copy link
Member

As part of the PR, it also fixes:

  • Removes button aliases
  • Unifies the keys in the object returned by getAllToolboxButtons and the button keys
  • Makes sure that the number of buttons displayed are always the same as the number of buttons specified in the thresholds and removes the exception for not filling up the main toolbar with buttons from overflow menu when reactions button is disabled.
  • Introduces a priority for buttons that will be used to fill empty spaces in the main toolbar.

@hristoterezov hristoterezov force-pushed the toolbox-custom-order branch 2 times, most recently from 2f10d97 to 357ee29 Compare May 17, 2024 14:58
@hristoterezov hristoterezov marked this pull request as ready for review May 20, 2024 16:38
@@ -440,6 +440,7 @@ export interface IConfig {
};
localSubject?: string;
locationURL?: URL;
mainToolbarButtons?: Array<Array<ToolbarButton | string>>;
Copy link
Member

Choose a reason for hiding this comment

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

Won't it always be an array of strings?

Copy link
Member Author

Choose a reason for hiding this comment

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

Generally yes, since ToolbarButton is ultimately a string. But when we add the ToolbarButton type this can improve the autocomplete in the IDE, you get some suggestions from ToolbarButton type. In addition it looks a little but clearer to me because it shows that you can put our stock buttons or a custom one (which is represented by | string).

If you don't like it I don't mind removing the ToolbarButton part tho.

Copy link
Member

Choose a reason for hiding this comment

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

Please do, since it doesn't match the actual config.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

if (isHangupVisible) {
sliceIndex -= 1;
}
// if we have 1 button in the overflow menu it is better to directly display it in the main toolbar by replacing
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

react/features/toolbox/constants.ts Show resolved Hide resolved
react/features/toolbox/types.ts Outdated Show resolved Hide resolved
saghul
saghul previously approved these changes May 23, 2024
As part of the PR, it also fixes:
 - Removes button aliases
 - Unifies the keys in the object returned by getAllToolboxButtons and the button keys
 - Makes sure that the number of buttons displayed are always the same as the number of buttons specified in the thresholds and removes the exception for not filling up the main toolbar with buttons from overflow menu when reactions button is disabled.
 - Introduces a priority for buttons that will be used to fill empty spaces in the main toolbar.
@hristoterezov hristoterezov merged commit 0913554 into master May 23, 2024
9 checks passed
@hristoterezov hristoterezov deleted the toolbox-custom-order branch May 23, 2024 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants