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

fix(LeftSidebar): get rid of server styles for settings button #12261

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ShGKme
Copy link
Contributor

@ShGKme ShGKme commented May 3, 2024

☑️ Resolves

  • Settings button is a nix of global legacy server styles and @nextcloud/vue
    • It's style is a bit different from other buttons
    • It has double outline on focus
  • Options 1: secondary - same as other similar apps - Mail, Calendar
  • Options 2: tertiary - close to previous design
  • Also, added a top padding

🖌️ UI Checklist

🖼️ Screenshots / Screencasts

🏚️ Before 🏡 After (secondary) 🏡 After (tertiary)
image image image
image image image
image . image

🏁 Checklist

  • 🌏 Tested with Chrome, Firefox and Safari or should not be risky to browser differences
  • 🖥️ Tested with Desktop client or should not be risky for it
  • 🖌️ Design was reviewed, approved or inspired by the design team
  • ⛑️ Tests are included or not possible
  • 📗 User documentation in https://github.com/nextcloud/documentation/tree/master/user_manual/talk has been updated or is not required

@ShGKme ShGKme added this to the 💙 Next Major (30) milestone May 3, 2024
@ShGKme ShGKme self-assigned this May 3, 2024
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
src/components/LeftSidebar/LeftSidebar.vue Show resolved Hide resolved
</NcButton>
</div>
<div class="left-sidebar__settings-button-container">
<NcButton type="tertiary" wide @click="showSettings">
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with current state, as it draws much less attention (and looks better 😋 ):

Talk Mail Calendar
image image image

Copy link
Member

Choose a reason for hiding this comment

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

Should this be handled in/via https://github.com/nextcloud-libraries/nextcloud-vue/blob/master/src/components/NcAppNavigationSettings/NcAppNavigationSettings.vue or a new component, to avoid all the custom code/handling in X apps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should this be handled in/via https://github.com/nextcloud-libraries/nextcloud-vue/blob/master/src/components/NcAppNavigationSettings/NcAppNavigationSettings.vue or a new component, to avoid all the custom code/handling in X apps?

I though about it but I'm not sure about it.

This cannot be handled via NcAppNavigationSettings (at least without making it a 2-in-1 component), because NcAppNavigationSettings is about "deprecated" collapsible settings, not a button.

This could be a new component, but its usage would probably have more variability than commons:

  • In some apps, AppNavigation is fulfilled with navigation items and the settings button should look like a navigation item (Files, Photos)
  • In some apps it would be secondary (Mail)
  • In some apps it would be tertiary (Talk)

The only common is that it is a wide NcButton.

But we can discuss it with designers in @nextcloud/vue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm fine with current state, as it draws much less attention (and looks better 😋 ):

The current state - in main or in this PR (tertiary)?

Copy link
Member

Choose a reason for hiding this comment

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

In some apps it would be secondary (Mail)
In some apps it would be tertiary (Talk)

This is exactly why I think it should be a common component.
I'm not sure the difference is intentional.

In fact I think that Talk, Calendar and Mail should look the same

Copy link
Contributor

@Antreesy Antreesy May 6, 2024

Choose a reason for hiding this comment

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

The current state - ... this PR (tertiary)

As discussed above 🙈, Talk and Mail use simple NcButton as a trigger for opening a dialog, and Calendar has special component with "accordion" feature (<button> !!)

So I'd say, it would be better to summon design team here, to come to a conclusion on appearance for all three apps (but not the functionality), then apply resulting styles on Mail and Vue-lib for Calendar.

Another approach could be to add #trigger slot in NcAppSettingsDialog and keep styles consistent for both components on library level

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

The accordion should go asap, in favor of the settings dialog with proper explanations.
I like both the secondary y and tertiary versions

Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants