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

refactor: make browserCheck an util instead of mixin #11009

Merged
merged 2 commits into from
Nov 27, 2023

Conversation

ShGKme
Copy link
Contributor

@ShGKme ShGKme commented Nov 27, 2023

☑️ Resolves

  • There is no need to be a mixin, it has nothing Vue-related or reactive
  • Having this module as a mixin:
    • Limits the usage to only Vue components
    • Reruns checks and all version detections each time the mixin is used (though it is guaranteed that the browser cannot be changed in runtime)
    • Adds many unneeded properties to component instance listed in devtools

🖌️ UI Checklist

🖼️ Screenshots / Screencasts

No visual changes

🚧 Tasks

  • Move the module browserCheck to utils
  • Refactor computed and Vue component methods to exported constants and functions
  • Update the usage in components
  • Move callButtonTooltipText to CallButton as it was supposed to be used only in CallButton

🏁 Checklist

  • 🌏 Tested with Chrome, Firefox and Safari or should not be risky to browser differences
  • 🖌️ Design was reviewed, approved or inspired by the design team, or there is no visual change
  • ⛑️ 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

- There is no need to be a mixin, it has nothing Vue-related or reactive
- Move to utils
- Refactor computed and Vue component methods to exported constants and functions

Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
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.

Flawless. Tested in Chrome + Firefox

One non-blocking comment:

src/utils/browserCheck.js Outdated Show resolved Hide resolved
- faisalman/ua-parser-js#599 is fixed in 1.0.32

Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
Copy link
Contributor

@SystemKeeper SystemKeeper left a comment

Choose a reason for hiding this comment

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

Awesome! Also tested with "real safari" works as well :)

@nickvergessen nickvergessen merged commit 7d47b43 into main Nov 27, 2023
36 checks passed
@nickvergessen nickvergessen deleted the refactor/browser-detection branch November 27, 2023 15:58
@SystemKeeper
Copy link
Contributor

/backport to stable28

@ShGKme
Copy link
Contributor Author

ShGKme commented Nov 27, 2023

Awesome! Also tested with "real safari" works as well :)

As I understood the problem, it was not reproducible in Safari on Mac. Only on specific old versions on iPad.

@SystemKeeper
Copy link
Contributor

/backport to stable27

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants