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

Reuse top bar component for responses view toggle #1576

Merged
merged 1 commit into from
Mar 23, 2024

Conversation

susnux
Copy link
Collaborator

@susnux susnux commented Mar 20, 2023

This is an alternative for #1559 to reuse the top bar menu. I split the TopBar component into TopBar and a reusable PillMenu component.

before after
225323334-b47231d2-8b4c-4779-af60-13195d6430dc image

Why?

  • Make the menu keyboard accessible
  • Consistent look

@susnux susnux added bug Something isn't working design Related to the design 3. to review Waiting for reviews labels Mar 20, 2023
@susnux susnux requested review from jancborchardt and jotoeri March 20, 2023 18:00
Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Awesome work @susnux! :) Only 2 things:

  • On the "after" screenshot, the overall border is overlapped by the slight blue background of the selected item. It would look better if the selected item is fully within the border. Like in the View/Edit/Results switcher, see "After" video of More usable and accessible navigation between View/Edit/Results #1381
  • It would probably be best to move this into the Vue components library directly? Cause I’d say this would look great for the tab navigation in the right sidebar as well, like in Files and Talk – what do you think @nimishavijay @marcoambrosini?

@jancborchardt
Copy link
Member

Also @susnux a similar component was discussed at nextcloud-libraries/nextcloud-vue#603 (comment), cc @raimund-schluessler for reference. :)

@susnux
Copy link
Collaborator Author

susnux commented Mar 31, 2023

* [ ]  On the "after" screenshot, the overall border is overlapped by the slight blue background of the selected item. It would look better if the selected item is fully within the border. Like in the View/Edit/Results switcher, see "After" video of [More usable and accessible navigation between View/Edit/Results #1381](https://github.com/nextcloud/forms/pull/1381)

This is currently broken (also on the view toggle), because NcContent introduced box-sizing: border-box so the height does include the border:
image

I fixed this with the latest commit.

* [ ]  It would probably be best to move this into the Vue components library directly? Cause I’d say this would look great for the tab navigation in the right sidebar as well, like in Files and Talk

Good idea, I will submit a PR soon.

@susnux
Copy link
Collaborator Author

susnux commented May 22, 2023

This will then require @nextcloud/vue 8. But the current beta release does not work, we need to wait for the next one (issue with the nccheckboxradioswitch is already fixed but not released).

@susnux susnux added the upstream Blocked or cause by an upstream issue label May 22, 2023
@susnux susnux added 2. developing Work in progress and removed 3. to review Waiting for reviews labels May 30, 2023
@susnux susnux marked this pull request as draft October 21, 2023 22:40
@susnux susnux force-pushed the feat/use-topbar-view-toggle branch from 86b8843 to 01e4e8f Compare March 22, 2024 12:22
@susnux susnux added 3. to review Waiting for reviews and removed 2. developing Work in progress upstream Blocked or cause by an upstream issue labels Mar 22, 2024
@susnux susnux added this to the 4.2 milestone Mar 22, 2024
@susnux susnux marked this pull request as ready for review March 22, 2024 12:23
@susnux susnux dismissed jancborchardt’s stale review March 22, 2024 12:23

changes implemented

@susnux
Copy link
Collaborator Author

susnux commented Mar 22, 2024

I have implemented the changes and switched to the checkboxradioswitch component.

Mobile:
Screenshot 2024-03-22 at 13-25-04 dsfwf - Forms - Nextcloud

Top bar:
Screenshot 2024-03-22 at 13-24-39 dsfwf - Forms - Nextcloud

Response view:
Screenshot 2024-03-22 at 13-24-50 dsfwf - Forms - Nextcloud


Also fixed on small mobile were previously the navigation was not accessible anymore (hidden outside the viewport) by now wrapping the top bar:

vokoscreenNG-2024-03-22_13-17-38.mp4

@Chartman123

This comment was marked as resolved.

Split TopBar into TopBar and reuseable PillMenu, and reuse for responses view selector.
The toggle is now based on the NcCheckboxRadioSwitch component

Signed-off-by: Ferdinand Thiessen <rpm@fthiessen.de>
@susnux susnux force-pushed the feat/use-topbar-view-toggle branch from 01e4e8f to 13e4b2b Compare March 22, 2024 18:26
@susnux

This comment was marked as resolved.

@Chartman123 Chartman123 merged commit fd0f8c8 into main Mar 23, 2024
43 checks passed
@Chartman123 Chartman123 deleted the feat/use-topbar-view-toggle branch March 23, 2024 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug Something isn't working design Related to the design
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants