-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add ImageLicense to SubjectPageVisualElement #567
Conversation
222dc66
to
93e2d2c
Compare
93e2d2c
to
2ed6345
Compare
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.
Litt små pirk
context: ContextWithLoaders, | ||
): Promise<GQLImageLicense | undefined> { | ||
const imageId = parseInt(visualElement.url.split("/").pop() ?? ""); | ||
if (isNaN(imageId)) return undefined; |
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.
Trenger vell strengt tatt ikke å sjekke om det er number? fetchImageV3 tar også imot string.
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.
Fjerna nu
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.
Visuelt element kan være en video, og då er url https://players.brightcove.net/4806596774001/BkLm8fT_default/index.html?videoId=6300755876001
Tenker det er greit å ha den sjekken likevel.
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.
Det vil jo ikke påvirke resultatet/callet til fetchImageV3
? Den sjekken eller parsingen er jo ikke nødvendig for bruken av metoden da vi henter bildet likegyldig om det er streng eller nummer.
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.
Det er sant, men eg vil anta at isNaN er kjappere enn et http-kall som feiler. Ingen grunn til å bruke meir tid enn nødvendig tenker eg.
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.
Da legger jeg det tilbake 😄
47c00a1
to
2bda83a
Compare
Necessary for NDLANO/Issues#4313