-
Notifications
You must be signed in to change notification settings - Fork 14
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
DMED-119 - Spike: direktes Einbinden der edu-sharing-Suchumgebung in den "Lern-Store" der SVS #2933
base: main
Are you sure you want to change the base?
Conversation
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
Quality Gate passedKudos, no new issues were introduced! 0 New issues |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are accessibility issues in these changes.
public/vendor/edu-sharing/assets/cordova/android/plugins/cordova-plugin-camera/www/Camera.js
Outdated
Show resolved
Hide resolved
Quality Gate failedFailed conditions See analysis details on SonarCloud Catch issues before they fail your Quality Gate with our IDE extension SonarLint |
@@ -254,7 +100,8 @@ watchDebounced( | |||
flex-direction: column; | |||
justify-content: space-between; | |||
width: 100%; | |||
min-height: calc(100vh - var(--sidebar-item-height)); | |||
min-height: calc(100vh - var(--sidebar-item-height) - 105.58px); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have to work with fixed values here?
<style scoped> | ||
.main-container { | ||
min-height: 0; | ||
flex-grow: 1; | ||
display: flex; | ||
flex-direction: column; | ||
gap: 20px; | ||
} | ||
.search-form { | ||
height: 64px; | ||
flex-shrink: 0; | ||
padding: 10px 16px; | ||
background: #f3f5f7; | ||
} | ||
.search-input { | ||
width: 500px; | ||
border-radius: 2px; | ||
border: 1px solid var(--Secondary-v-secondary-base, #54616e); | ||
background: var(--shades---v-white-base, #fff); | ||
} | ||
edu-sharing-app { | ||
flex-grow: 1; | ||
} | ||
:deep(.v-field) { | ||
height: 44px; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
muss so viel custom css sein?
.main-container { | ||
min-height: 0; | ||
flex-grow: 1; | ||
display: flex; | ||
flex-direction: column; | ||
gap: 20px; | ||
} | ||
.search-form { | ||
height: 64px; | ||
flex-shrink: 0; | ||
padding: 10px 16px; | ||
background: #f3f5f7; | ||
} | ||
.search-input { | ||
width: 500px; | ||
border-radius: 2px; | ||
border: 1px solid var(--Secondary-v-secondary-base, #54616e); | ||
background: var(--shades---v-white-base, #fff); | ||
} | ||
edu-sharing-app { | ||
flex-grow: 1; | ||
} | ||
:deep(.v-field) { | ||
height: 44px; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Warum so viel eigene styles?
@@ -39,15 +50,25 @@ const createH5pEditorProxy = () => { | |||
return h5pEditorProxy; | |||
}; | |||
|
|||
const createEduSharingRepoProxy = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Warum benötigt man ein eduSharingProxy und ein eduSharingRepoProxy? Was ist der Unterschied zwischen beiden?
@@ -73,6 +98,20 @@ const createDevServerConfig = () => { | |||
}, | |||
}); | |||
|
|||
// Copy assets before the dev server starts | |||
const __base = path.resolve(__dirname, "../.."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Es wäre super wenn der Code dafür noch unter einer Funktion mit bezeichnenden Namen geclustert wird. Ehrlich gesagt müsste ich aber noch mal über das warum es notwendig ist nachdenken. Bzw. hier wäre drüber reden hilfreich.
Sind die ganzen Änderung potentiel etwas das hinter ein feature flag liegen sollten?
event.preventDefault(); | ||
const data = new FormData(event.target); | ||
const searchString = data.get("searchString"); | ||
const eduSharingApp = document.getElementsByTagName("edu-sharing-app")[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
document.getElementsByTagName("edu-sharing-app")[0] = erstes Element von irgendwas externen klinkt jetzt erstmal nicht nach Langzeit Maintaince save.
document.body.appendChild(styles); | ||
|
||
// retrieve the valid session ticket | ||
(async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ich kann dir nicht sagen wie es aussehen muss. Aber die api zur Komponente zu importieren und beim mount ein async einzubauen klinkt erstmal nicht richtig.
envConfigModule.getEnv.EDU_SHARING__API_URL + "/rest", | ||
}; | ||
|
||
const runtime = document.createElement("script"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
createElement
setAttribut
appendChild
etc.. in einer vue Komponente klinkt nicht nach state of the art.
} from "@/eduSharingApi/v3"; | ||
import { $axios } from "@/utils/api"; | ||
|
||
export const useEduSharingApi = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nur so als Frage sollte es ein composal geben das -api heißt?
Ich würde erwarten das ein composal maximal fachlichkeit ausdrückt und das es eine Datenquelle dahinter gibt, kann sein muss aber nicht, selbst die Art der Quelle ob Browser Cookie, json, api request dahinter liegt.
Evtl. ist es nur das umbennen des Files.
/* tslint:disable */ | ||
/* eslint-disable */ | ||
/** | ||
* HPI Schul-Cloud Server API |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Das hatten wir doch schon im backend.
Ich glaube das sollte SVS Backend API sein. Wobei das ja eigentlich eduSharing ist.
/* eslint-disable */ | ||
/** | ||
* HPI Schul-Cloud Server API | ||
* This is v3 of HPI Schul-Cloud Server. Checkout /docs for v1. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hier das gleiche Minus HPI, + SVS
/* tslint:disable */ | ||
/* eslint-disable */ | ||
/** | ||
* HPI Schul-Cloud Server API |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
/* tslint:disable */ | ||
/* eslint-disable */ | ||
/** | ||
* HPI Schul-Cloud Server API |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
/* eslint-disable */ | ||
/** | ||
* HPI Schul-Cloud Server API | ||
* This is v3 of HPI Schul-Cloud Server. Checkout /docs for v1. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and again
</transition> | ||
</div> | ||
<content-edu-sharing-footer class="content__footer" /> | ||
<EduSharingWrapper /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Big bang, oder feature flag?
Was ist mit sowas wie stores die evtl. noch rum liegen?
Quality Gate failedFailed conditions |
Short Description
The aim of this spike is to evaluate how the edu-sharing search environment can best be integrated as a "learning store" within the school cloud network software.
Links to Ticket and related Pull-Requests
https://ticketsystem.dbildungscloud.de/browse/DMED-119
hpi-schul-cloud/dof_app_deploy#713
hpi-schul-cloud/schulcloud-server#4692
hpi-schul-cloud/schulcloud-client#3387
hpi-schul-cloud/oeh-search-etl#50
Links to deployments (dev environments) :
https://dmed-119-integration-of-search-environment.dbc.dbildungscloud.dev/
https://dmed-119-integration-of-search-environment.nbc.dbildungscloud.dev/
https://dmed-119-integration-of-search-environment.brb.dbildungscloud.dev/
Link to deployments (in test environment) :
https://test.mediathek.dev.dbildungsplattform.de/
Changes
Data-security
Deployment
New Repos, NPM packages or vendor scripts
Screenshots of UI changes
Checklist before merging