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.

Sign up for GitHub

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

Jump to bottom

self-built app banner #9696

Merged
merged 16 commits into from
Sep 28, 2023
Merged

self-built app banner #9696

merged 16 commits into from
Sep 28, 2023

Conversation

grimmoc
Copy link
Contributor

@grimmoc grimmoc commented Sep 15, 2023 •
edited by fschade
Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

  • test environment:
  • test case 1:
  • test case 2:
  • ...

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Open tasks:

  • ...

@update-docs
Copy link

update-docs bot commented Sep 15, 2023

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

dschmidt
dschmidt reviewed Sep 15, 2023
View reviewed changes
Copy link
Member

@dschmidt dschmidt left a comment

Choose a reason for hiding this comment

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

cool, like this much more than the previous implementation with smartbanner.js

packages/web-app-files/src/views/AppBanner.vue Outdated Show resolved Hide resolved
packages/web-app-files/src/views/AppBanner.vue Outdated Show resolved Hide resolved
packages/web-app-files/src/views/AppBanner.vue Outdated Show resolved Hide resolved
packages/web-app-files/src/views/AppBanner.vue Outdated Show resolved Hide resolved
packages/web-app-files/src/views/AppBanner.vue Outdated Show resolved Hide resolved
@grimmoc grimmoc marked this pull request as ready for review September 25, 2023 09:37
dschmidt
dschmidt previously requested changes Sep 25, 2023
View reviewed changes
packages/web-app-files/src/views/AppBanner.vue Outdated Show resolved Hide resolved
packages/web-app-files/src/views/AppBanner.vue Outdated Show resolved Hide resolved
packages/web-app-files/src/views/AppBanner.vue Outdated Show resolved Hide resolved
packages/web-app-preview/src/App.vue Outdated Show resolved Hide resolved
packages/web-runtime/package.json Outdated Show resolved Hide resolved
packages/web-runtime/themes/owncloud/theme.json Show resolved Hide resolved
pnpm-lock.yaml Outdated Show resolved Hide resolved
@fschade
Copy link
Contributor

fschade commented Sep 25, 2023 •
edited
Loading

@grimmoc, nice, thanks for you PR, some nitpick's from my side:

  • @dschmidt took his time to provide feedback, there still some unresolved things, please fix or answer them.
  • Please add some information to this pr, just follow the pr template, the reason for this, we reference the pr id in the changelog, and therefor we need to have a quick elevator description.
  • please rebase, i have the feeling base is a bit outdated
  • please add some tests

let me know if you need help bringing this over the finishing line, we can pair on that. I can show you how to run the tests locally, saves some time ;)

@grimmoc
Copy link
Contributor Author

grimmoc commented Sep 25, 2023

@grimmoc, nice, thanks for you PR, some nitpick's from my side:

* @dschmidt took his time to provide feedback, there still some unresolved things, please fix or answer them.

* Please add some information to this pr, just follow the pr template, the reason for this, we reference the pr id in the changelog, and therefor we need to have a quick elevator description.

* please rebase, i have the feeling base is a bit outdated

* please add some tests

let me know if you need help bringing this over the finishing line, we can pair on that. I can show you how to run the tests locally, saves some time ;)

most of the things @dschmidt mentioned are resolved now. rebasing was virtually done today. will check out adding tests if necessary.

@fschade fschade changed the title wip self-built app banner self-built app banner Sep 27, 2023
JammingBen
JammingBen reviewed Sep 27, 2023
View reviewed changes
Copy link
Contributor

@JammingBen JammingBen left a comment

Choose a reason for hiding this comment

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

Some last minor inputs from my side, almost finished 🥳 Did you have a chance to ask @tbsbdr regarding the additionalInformation?

packages/web-app-preview/src/App.vue Outdated Show resolved Hide resolved
packages/web-app-preview/src/App.vue Outdated Show resolved Hide resolved
packages/web-pkg/src/components/AppBanner.vue Outdated Show resolved Hide resolved
@grimmoc
Copy link
Contributor Author

grimmoc commented Sep 27, 2023

Some last minor inputs from my side, almost finished 🥳 Did you have a chance to ask @tbsbdr regarding the additionalInformation?

yes, we leave it in but make it empty by default.

@sonarcloud
Copy link

sonarcloud bot commented Sep 27, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot E 1 Security Hotspot
Code Smell A 3 Code Smells

91.7% 91.7% Coverage
0.0% 0.0% Duplication

JammingBen
JammingBen approved these changes Sep 28, 2023
View reviewed changes
Copy link
Contributor

@JammingBen JammingBen left a comment

Choose a reason for hiding this comment

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

🚀

@JammingBen JammingBen requested a review from dschmidt September 28, 2023 06:21
AlexAndBear
AlexAndBear approved these changes Sep 28, 2023
View reviewed changes
packages/web-pkg/src/components/AppBanner.vue
export default defineComponent({
components: {},
props: {
fileId: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not quite sure if this should be always required, as a generic component this could be shown even if not public link / file context

@JammingBen JammingBen dismissed dschmidt’s stale review September 28, 2023 07:48

Changes have been addressed

@grimmoc grimmoc merged commit baef9d6 into master Sep 28, 2023
2 checks passed
@delete-merged-branch delete-merged-branch bot deleted the mobile-app-banner branch September 28, 2023 07:54
@fschade
Copy link
Contributor

fschade commented Sep 29, 2023

🎈

@micbar micbar mentioned this pull request Dec 12, 2023
Merged
12 tasks
AlexAndBear pushed a commit that referenced this pull request Dec 13, 2023
@grimmoc @AlexAndBear
self-built app banner (#9696)
bd9531a 
* wip self-built app banner

* wip almost complete

* complete, final tests pending

* fixed URL generation, works!

* clean up

* added changelog

* fixed pnpm lock

* linting

* fixed test

* changes per request in pr

* added tests, moved component to web pkg

* minor cosmetic changes

* changes per request from PO

* added custom translation hack

* fixed after rebase

* removed console log
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JammingBen JammingBen JammingBen approved these changes

@AlexAndBear AlexAndBear AlexAndBear approved these changes

@dschmidt dschmidt Awaiting requested review from dschmidt

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[web] Open in web or app banner
5 participants
@grimmoc @fschade @dschmidt @AlexAndBear @JammingBen