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

UU-Disclaimer embed updates #2659

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Conversation

katrinewi
Copy link
Contributor

@katrinewi katrinewi commented Dec 13, 2024

https://github.com/NDLANO/Issues/issues/4245

relatert: NDLANO/graphql-api#528

Tilhørende PR i ed kommer også

@katrinewi katrinewi requested a review from a team December 13, 2024 14:03
@katrinewi katrinewi force-pushed the disclaimer-embed-updates branch from 28c4c3d to e842a11 Compare December 13, 2024 14:15
@katrinewi katrinewi force-pushed the disclaimer-embed-updates branch from e842a11 to 02a477e Compare December 13, 2024 14:19
@katrinewi katrinewi requested a review from Jonas-C December 13, 2024 14:44
@katrinewi katrinewi force-pushed the disclaimer-embed-updates branch from 6bb2f0c to 7e501a6 Compare December 13, 2024 14:45
@katrinewi katrinewi force-pushed the disclaimer-embed-updates branch from 17b695b to 8c15da5 Compare December 17, 2024 08:00
@gunnarvelle
Copy link
Member

Kan sikkert ta inn denne og publisere pakker. Eksisterende kode vil jo ikkje påvirkes av nye versjoner her.

@@ -15,7 +15,10 @@ import { type PluginType } from "../types";
export const uuDisclaimerEmbedPlugin: PluginType = (element, opts, transformOpts) => {
const props = attributesToProps(element.attribs);
const data = JSON.parse(props["data-json"] as string) as UuDisclaimerMetaData;
const transformedDisclaimer = transform(data.embedData.disclaimer, transformOpts);
if (data.status === "error") {
return null;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Burde vi ha bedre errorhåndtering her? vurderte å legge til støtte for uu-disclaimer i EmbedErrorPlaceholder, innspill?

Copy link
Contributor

Choose a reason for hiding this comment

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

Synes det høres lurt ut, jeg. Evt bare vise innhold direkte?

@katrinewi katrinewi requested a review from Jonas-C January 2, 2025 18:34
@katrinewi katrinewi force-pushed the disclaimer-embed-updates branch from 08ce135 to abf854d Compare January 2, 2025 19:01
const props = attributesToProps(element.attribs);
const data = JSON.parse(props["data-json"] as string) as UuDisclaimerMetaData;
return <UuDisclaimerEmbed embed={data}>{domToReact(element.children as DOMNode[], opts)}</UuDisclaimerEmbed>;
if (data.status === "error") {
return <>{domToReact(element.children as DOMNode[], opts)}</>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Trenger vi et fragment her?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Jepp, ellers blir TS sinna pga return typen til funksjonen

},
});

const UuDisclaimerEmbed = ({ embed, children }: Props) => {
const UuDisclaimerEmbed = ({ embed, transformedDisclaimer, children }: Props) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Vil vi fortsatt returnere ingenting dersom disclaimer har feil?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm hva med å returnere innholdet også vise en feilmelding over eller noe?

Comment on lines +50 to +51
aria-label={t("uuDisclaimer.title")}
title={t("uuDisclaimer.title")}
Copy link
Contributor

Choose a reason for hiding this comment

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

Jeg mistenker at disse vil bli satt av PopoverTitle automatisk. Kan det stemme?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Jeg tror ikke det, ser dårlig ut

@katrinewi katrinewi requested a review from Jonas-C January 8, 2025 08:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants