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(sidebar): use open state to remove always working focus trap on mobile #12387

Merged
merged 2 commits into from
Jun 28, 2024

Conversation

ShGKme
Copy link
Contributor

@ShGKme ShGKme commented May 17, 2024

☑️ Resolves

🖌️ UI Checklist

🖼️ Screenshots / Screencasts

🏚️ Before 🏡 After
image image

🚧 Tasks

  • Style changes

🏁 Checklist

@ShGKme ShGKme added this to the 💙 Next Major (30) milestone May 17, 2024
@ShGKme ShGKme self-assigned this May 17, 2024
@ShGKme ShGKme changed the title fix(sidebar): use open state to remove always working focus trap fix(sidebar): use open state to remove always working focus trap with @nextcloud/vue@8.13.0 Jun 13, 2024
@ShGKme ShGKme changed the title fix(sidebar): use open state to remove always working focus trap with @nextcloud/vue@8.13.0 fix(sidebar): use open state to remove always working focus trap on mobile with @nextcloud/vue@8.13.0 Jun 13, 2024
@ShGKme ShGKme changed the title fix(sidebar): use open state to remove always working focus trap on mobile with @nextcloud/vue@8.13.0 fix(sidebar): use open state to remove always working focus trap on mobile Jun 13, 2024
@ShGKme ShGKme added the dependencies Pull requests that update a dependency file label Jun 13, 2024
@Antreesy Antreesy force-pushed the fix/app-sidebar-open branch 2 times, most recently from 3e745d0 to 2714865 Compare June 25, 2024 16:36
Copy link
Contributor

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

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

Works as expected. Though there are still too many styles changes came from library (follow-up?)

@Antreesy Antreesy marked this pull request as ready for review June 25, 2024 16:38
@ShGKme
Copy link
Contributor Author

ShGKme commented Jun 25, 2024

Though there are still too many styles changes came from library (follow-up?)

What do you mean? Are they from the library or new server styles?

@ShGKme
Copy link
Contributor Author

ShGKme commented Jun 25, 2024

@nextcloud/vue@8 doesn't suppose to have any breaking change at least for supported 28 and 29.

Styles on 30 are broken in many places also from server styles, it's in progress

@Antreesy
Copy link
Contributor

Yeah, server styles, I have compared at main only. Tested with desktop + server 29, so far looks the same

@ShGKme ShGKme marked this pull request as draft June 26, 2024 19:51
@ShGKme ShGKme marked this pull request as ready for review June 26, 2024 20:44
@ShGKme
Copy link
Contributor Author

ShGKme commented Jun 26, 2024

  • Rebased onto main
  • Minor fixes in fixup:
    • Correctly use conditional slot
    • Pass all the previous attrs to the button like title and aria-label.

@ShGKme
Copy link
Contributor Author

ShGKme commented Jun 27, 2024

  • Rebased onto main to resolve conflicts
  • Fixed styles. Required, because with new Teleport fix in NcAppSidebar, the toggle is no longer inside the sidebar

image

@ShGKme
Copy link
Contributor Author

ShGKme commented Jun 27, 2024

Note, screenshot is made on stable29 styles

Copy link
Contributor

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

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

Tested against stable29

on main ubread marker is still messy, but because of reducing the clickable area 44 -> 34

@DorraJaouad
Copy link
Contributor

On main :
message (shouldn't be lower ? It doesn't look much readable)
image
mention (at all)
image
direct mention
image

Color should be applied only on message notif

.chat-button__unread-messages-counter {
	color: var(--color-primary-element);
}

Besides, styles should be aligned with conversation counter styles ?

@ShGKme
Copy link
Contributor Author

ShGKme commented Jun 27, 2024

On main

@DorraJaouad Is it much different from what was before? Styles on main, including @nextcloud/vue are not adjusted to new theming.

Antreesy and others added 2 commits June 28, 2024 11:03
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
@Antreesy
Copy link
Contributor

Rebased on main, squashed fixups and provide a fix for 'highlighted' type:
image

Copy link
Contributor

@DorraJaouad DorraJaouad left a comment

Choose a reason for hiding this comment

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

Tested

@Antreesy Antreesy merged commit 1da2c9f into main Jun 28, 2024
46 checks passed
@Antreesy Antreesy deleted the fix/app-sidebar-open branch June 28, 2024 09:34
@ShGKme
Copy link
Contributor Author

ShGKme commented Jun 28, 2024

/backport to stable29

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review bug dependencies Pull requests that update a dependency file feature: frontend 🖌️ "Web UI" client
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't select message text box
3 participants