refactor: make internal editor code more generic#3344
refactor: make internal editor code more generic#3344ahtesham-quraish wants to merge 14 commits into
Conversation
OpenAPI ChangesNo changes detected Unexpected changes? Ensure your branch is up-to-date with |
ahtesham-quraish
changed the title
ahtesham-quraish
force-pushed
the
ahtesham/tiptap-refactor
branch
7 times, most recently
from
f462266 to
69b6536
Compare
ChristopherChudzicki
left a comment
ChristopherChudzicki
left a comment
There was a problem hiding this comment.
@ahtesham-quraish I haven't actually tried creation/editing flows on this branch yet, but some feedback on the code is below.
TipTap Editor Organization
Overall, I think we need to strive to better separate news + article. Currently, we've created the /TiptapEditor/contentTypes directory with article and news, but:
newsExtensions= everythingarticleExtensions= just alias of news
We should think about how extensions group:
| Tier | Criterion | Where | Examples |
|---|---|---|---|
| 1 | Every content type needs it (now & likely future) and no per-type config | extensions/baseExtensions.ts — one factory |
StarterKit, Placeholder, Heading, Image*, Divider, MediaEmbed*, LearningResource*, … |
| 2 | Shared, but takes per-type config | extensions/node/*, composed into each type's list |
Banner (breadcrumb, background, dark/light), Byline (share-URL prefix — hardcoded /news/ today, ArticleByLineInfoBar.tsx:120) |
| 3 | Specific to one content type | contentTypes/<type>/ |
Document expression, seed doc, the per-type config object |
Tier-1 test: constructible from only the (uploadHandler, setUploadError) runtime deps, no type-specific arg. Don't pre-split it for hypothetical future exclusions. Net effect: each create<Type>Extensions collapses to [ <Type>Document, ...createBaseExtensions(deps), Banner, Byline ] + seed + config.
All the generic extensions should go somewhere common, something like, maybe inside /TiptapEditor/extensions/baseExtensions.ts.
Other notes:
ArticleByLineInfoBarthis should be renamed; its name dates from the olden days when we called news pages were called articles. I think it could just beByLineInfoBar?- Re
GenericEditor: Minor, but Copilot suggested calling thisWebsiteContentEditor... it's not truly generic, it is specific to website content. So i think that's a good name. ArticleProvider/useArticlelook like naming debt;WebsiteContentProvider/useWebsiteContentwould better match the actualWebsiteContentpayload.
Regarding NextJS Routes
You added a bunch of new NextJS routes... e.g., /articles, /articles/new, etc. If we continue with that pattern, every time we add a new content_type, we will need to add a whole bunch of new route files.
I see two groups of routes:
- User Facing:
/news,/news/:id,/articles,/articles/:id... these are user-facing; adding new route files seems reasonabl - Internal for editing: the
/new,/[id]/edit,/draftsroutes.
I think we should make the editing routes generic: /website_content/[type]/new, /website_content/[type]/[id-or-slug]/edit, /website_content/drafts?content_type=whatever routes for editing call content types.
That way, when we add new content types, we don't need to add new editing routes.
An issue with redirect.
Did you try the new /articles, /articles/new, etc routes that you added? These currently redirect to /news, due to our nextjs config. (From https://github.com/mitodl/hq/issues/10324)
The website_content routes suggested above avoid the issue, at least for editing URLs.
We'll need to check in with Ferdi about whether or not we can remove the redirect. My recollection is that we added it because things were published at the old /articles/ URL before we were expecting. I'll check data tomorrow. It could be that if no one is visiting that page, we can get rid of it.
ahtesham-quraish
force-pushed
the
ahtesham/tiptap-front-branch
branch
from
43b42c0 to
f54a776
Compare
ahtesham-quraish
force-pushed
the
ahtesham/tiptap-front-branch
branch
from
f54a776 to
6087067
Compare
|
@ChristopherChudzicki I have applied the styles for the articles page you should check it too please. 
|
ChristopherChudzicki
left a comment
ChristopherChudzicki
left a comment
There was a problem hiding this comment.
@ahtesham-quraish The extension separation is much cleaner / clearer now. Thanks!
I didn't get too far with manually testing the flows, but I am seeing some code issues still. I can revisit manual testing first thing in the morning.
next.config.jsredirects ... We can remove these. Per https://github.com/mitodl/hq/issues/11365, we are ok to remove them, and will coordiante with Devops to remove the fastly redirects, too.- Naming: Lots of things still reference article where they shouldn't:
ArticleContext.tsx: This file should be WebsiteContentContext.tsx, right? Let's do a real rename rather than alias + deprecated flag. Just rename it. It's simpler. Diff is a little larger, but less confusion.- Same comment for everything in the
TiptapEditor/extensions/node/ArticleByLineInfoBardirectory. GenericEditor: Similarly, statements likeimport { WebsiteContentEditor } from "../../core/GenericEditor"are confusing. Don't deprecate + alias. Just rename it.- Doubly so for this file. It's completely new.
ArticleEditor.tsx: There are two ArticleEditor.tsx files. This is confusing.- As far as I can tell,
TiptapEditor/ArticleEditor.tsxis really the News editor, and exists only for the sake of its sibling,ArticleEditor.happydom.test.tsxwhich, similarly, is really testing the newseditor.- At the very least, let's get rid of
TiptapEditor/ArticleEditor.tsxand move the happydom test to be a sibling of NewsEditor.
- At the very least, let's get rid of
- As far as I can tell,
TiptapEditor/useArticleSchema.ts: This seems unused. Just get rid of it?
- ByLine ... I'm noticing the the articles byline shows a Share URL pointing to news, see screenshot below. It doesn't seem to be visible in the published url, just on the editing view? We should have the correct url though.
- Editing URLS: Good that /articles/new, etc, are thin wrappers of the website content versiosn. IMO, thogh, we should just drop
/articles/newand/news/newentirely. May want to confirm with @Ferdi , though.
|
@ahtesham-quraish One more note: I think this branch needs a rebase against your other branch, or against main after that is merged. that should fix the failing openapi thing. |
ahtesham-quraish
force-pushed
the
ahtesham/tiptap-refactor
branch
from
5887609 to
44179d8
Compare
ahtesham-quraish
force-pushed
the
ahtesham/tiptap-front-branch
branch
from
969c22a to
d51df58
Compare
ahtesham-quraish
force-pushed
the
ahtesham/tiptap-front-branch
branch
from
8bb5ac7 to
7d25eb8
Compare
There was a problem hiding this comment.
Pull request overview
Refactors the Tiptap-based website content editor so the shared editor “shell” (save/publish UI, uploads, schema validation, toolbar plumbing, learning-resource drawer) can be reused across multiple website content types, while keeping the existing /news editor behavior and introducing a new /articles surface backed by the same API.
Changes:
- Introduces a generic
WebsiteContentEditorcore and shared/base Tiptap extension factory, with per-content-type extension/config modules. - Adds content-type routed editor pages under
/website_content/...and wires/news/*and/articles/*to those routes (mostly via redirects). - Extends backend website content plumbing for the new
articletype (e.g., CDN purge listing URL mapping) and adjusts Celery task naming for compatibility.
Reviewed changes
Copilot reviewed 42 out of 42 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| website_content/views.py | Formatting-only cleanup in viewset. |
| website_content/models.py | Formatting-only import spacing. |
| website_content/models_test.py | Formatting-only import spacing. |
| website_content/migrations/0003_add_editors_group.py | Formatting-only docstring spacing. |
| website_content/factories.py | Formatting-only import spacing. |
| website_content/api.py | Adds listing URL mapping for article content type during CDN purge. |
| website_content/api_test.py | Formatting-only import spacing. |
| news_events/tasks.py | Adds/adjusts Celery tasks/aliases for website-content-driven news ETL and backward compatibility. |
| news_events/etl/articles_news.py | Minor formatting while syncing website content news to news feed. |
| frontends/main/src/page-components/TiptapEditor/useArticleSchema.ts | Switches legacy “article schema” hook to use the new news content-type extensions (with deprecated alias exports). |
| frontends/main/src/page-components/TiptapEditor/TiptapEditor.tsx | Updates viewer node mapping to allow configurable banner/byline viewers; adds a new styling hook. |
| frontends/main/src/page-components/TiptapEditor/index.ts | Re-exports new generic editor + content-type editors (plus aliases for compatibility). |
| frontends/main/src/page-components/TiptapEditor/extensions/node/Banner/ArticleBannerNode.tsx | Adds an article-specific banner node + viewer implementation. |
| frontends/main/src/page-components/TiptapEditor/extensions/node/ArticleByLineInfoBar/ArticleByLineInfoBarViewer.tsx | Renames/export-aliases byline viewer and adds a banner-specific byline viewer. |
| frontends/main/src/page-components/TiptapEditor/extensions/node/ArticleByLineInfoBar/ArticleByLineInfoBarNode.ts | Deprecates old node name and introduces ByLineInfoBarNode alias. |
| frontends/main/src/page-components/TiptapEditor/extensions/node/ArticleByLineInfoBar/ArticleByLineInfoBarInBanner.tsx | Adds a simplified byline display component for the banner area. |
| frontends/main/src/page-components/TiptapEditor/extensions/node/ArticleByLineInfoBar/ArticleByLineInfoBar.tsx | Renames exports (byline content/view) and updates context usage. |
| frontends/main/src/page-components/TiptapEditor/extensions/baseExtensions.ts | Introduces shared “base” Tiptap extensions factory for all content types. |
| frontends/main/src/page-components/TiptapEditor/core/GenericEditor.tsx | Adds the generic editor shell (WebsiteContentEditor) with save/publish UI, uploads, schema validation, and read-only viewer wiring. |
| frontends/main/src/page-components/TiptapEditor/contentTypes/news/newsExtensions.ts | Adds news-specific document schema + extension factory and initial doc. |
| frontends/main/src/page-components/TiptapEditor/contentTypes/news/NewsEditor.tsx | Adds news editor composition using the generic shell + news config and mutations. |
| frontends/main/src/page-components/TiptapEditor/contentTypes/article/articleExtensions.ts | Adds article-specific document schema + extension factory and initial doc. |
| frontends/main/src/page-components/TiptapEditor/contentTypes/article/ArticleEditor.tsx | Adds /articles editor composition using the generic shell + article config (including article banner viewer). |
| frontends/main/src/page-components/TiptapEditor/ArticleEditor.tsx | Replaces the old monolithic editor with a backward-compatible re-export to the new news editor. |
| frontends/main/src/page-components/TiptapEditor/ArticleContext.tsx | Generalizes article context to WebsiteContentContext with deprecated aliases. |
| frontends/main/src/common/urls.ts | Adds URL helpers/constants for /articles and generic /website_content edit/create/drafts routes. |
| frontends/main/src/app/website_content/drafts/page.tsx | Adds drafts route under /website_content/drafts. |
| frontends/main/src/app/website_content/[type]/new/page.tsx | Adds content-type parameterized “new content” route. |
| frontends/main/src/app/website_content/[type]/[idOrSlug]/edit/page.tsx | Adds content-type parameterized “edit content” route. |
| frontends/main/src/app/articles/page.tsx | Adds /articles listing route. |
| frontends/main/src/app/articles/new/page.tsx | Redirects /articles/new to the generic website content “new” route. |
| frontends/main/src/app/articles/draft/page.tsx | Redirects /articles/draft to the generic drafts listing with content_type=article. |
| frontends/main/src/app/articles/[slugOrId]/page.tsx | Adds /articles/:slugOrId detail route with SSR hydration + metadata generation. |
| frontends/main/src/app/articles/[slugOrId]/edit/page.tsx | Redirects /articles/:slugOrId/edit to the generic website content editor route. |
| frontends/main/src/app/articles/[slugOrId]/draft/page.tsx | Adds restricted draft view route for articles. |
| frontends/main/src/app/(site)/news/new/page.tsx | Redirects legacy /news/new to the generic website content “new” route for news. |
| frontends/main/src/app/(site)/news/draft/page.tsx | Redirects legacy /news/draft to the generic drafts listing with content_type=news. |
| frontends/main/src/app/(site)/news/[slugOrId]/edit/page.tsx | Redirects legacy /news/:slugOrId/edit to the generic website content editor route. |
| frontends/main/src/app-pages/WebsiteContent/WebsiteContentNewPage.tsx | Implements type-driven “new content” page that selects the correct editor composition. |
| frontends/main/src/app-pages/WebsiteContent/WebsiteContentEditPage.tsx | Implements type-driven “edit content” page that loads content and selects the correct editor composition. |
| frontends/main/src/app-pages/WebsiteContent/WebsiteContentDraftListingPage.tsx | Adds a unified drafts listing page intended to support filtering by content type. |
| frontends/main/src/app-pages/UserArticles/UserArticleNewPage.tsx | Adds a user-article “new” page using the article editor composition. |
| frontends/main/src/app-pages/UserArticles/UserArticleListingPage.tsx | Adds an /articles listing UI backed by useArticleList. |
| frontends/main/src/app-pages/UserArticles/UserArticleEditPage.tsx | Adds an edit page for articles backed by useArticleDetailRetrieve. |
| frontends/main/src/app-pages/UserArticles/UserArticleDraftListingPage.tsx | Adds a draft listing UI for articles backed by useArticleList({ draft: true }). |
| frontends/main/src/app-pages/UserArticles/UserArticleDetailPage.tsx | Adds a read-only article detail renderer using the article editor composition. |
Comments suppressed due to low confidence (1)
news_events/tasks.py:66
- The module-level reassignment
get_articles_news = get_website_content_newsshadows the@app.task(name="news_events.tasks.get_articles_news")task defined just above. This is redundant and makes imports/calls toget_articles_newsuse the new task name (not the backward-compatible one). Prefer removing this reassignment and rely on the explicitly-named Celery task for backward compatibility.
| content: JSONContent | ||
| extensions: Array<Extension | Node | Mark> | ||
| bannerViewer?: typeof BannerViewer | ||
| bylineViewer?: typeof BannerViewer |
| }, | ||
| } | ||
| : {}), | ||
| "&& .tiptap.ProseMirror.abc > :nth-child(2)": { |
| article?: WebsiteContent | ||
| backgroundColor?: string | ||
| bannerViewer?: typeof BannerViewer | ||
| bylineViewer?: typeof BannerViewer |
| const { create: createMutation, update: updateMutation } = saveMutations | ||
| const isPending = createMutation.isPending || updateMutation.isPending | ||
| const saveError = createMutation.error || updateMutation.error | ||
|
|
||
| const uploadImage = useMediaUpload() | ||
| // Keep a ref so the stable uploadHandler callback always calls the latest mutation. | ||
| const uploadImageRef = useRef(uploadImage) | ||
| uploadImageRef.current = uploadImage | ||
|
|
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| const listParams: any = { | ||
| limit: PAGE_SIZE, | ||
| offset: (page - 1) * PAGE_SIZE, | ||
| draft: true, | ||
| ...(contentType ? { content_type: contentType } : {}), |
| const [page, setPage] = useState(1) | ||
| const scrollRef = useRef<HTMLDivElement>(null) | ||
|
|
||
| const { data: articles, isLoading } = useArticleList({ |
There was a problem hiding this comment.
This has been refactored and this file is removed
| const { data: articles, isLoading: isLoadingArticles } = useArticleList({ | ||
| limit: PAGE_SIZE, | ||
| offset: (page - 1) * PAGE_SIZE, | ||
| draft: true, |
| addNodeView() { | ||
| // eslint-disable-next-line react-hooks/rules-of-hooks | ||
| return ReactNodeViewRenderer(ArticleByLineInfoBar) |
| import { useWebsiteContent } from "../../../ArticleContext" | ||
| import { calculateReadTime } from "../../utils" | ||
| import SharePopover from "@/components/SharePopover/SharePopover" |
ahtesham-quraish
force-pushed
the
ahtesham/tiptap-front-branch
branch
from
ec203c7 to
5496f48
Compare
ahtesham-quraish
force-pushed
the
ahtesham/tiptap-front-branch
branch
from
ac90e20 to
d55f623
Compare
There was a problem hiding this comment.
I'm getting lost lately with so many comments, so I'm going to put mine all in one comment, hopefully with enough context / links.
As discussed on zoom, still seeing a lot of naming issues. I know you're starting to address some of them already. Also a few other bugs.
Re Naming: We can't use article where we mean generic website content or news. article is specifically the content_type="article" from now one. This should apply everywhere. Otherwise, we end up with weird things like app-pages/Articles (which is really news) and app-pages/UserArticles which is really articles.
Naming
-
Hooks: discussed on zoom;
frontends/api/src/hooks/articlesshould bewebsite_contentnow. I don't think they ever need to mention article or news explicitly. -
app-pages/Articlesand**app-pages/UserArticlesThese are abot "news" and "article" content types, respectively, right? They should be named that way. Additionally, I think both of these directories should have much fewer files (See next item). -
Suggested File Structure: This PR has separate ArticleDetailPage and NewsDetailPage files (named differently, per Item 2 above). But they are nearly identical: They just render the editor in read-only mode. And that should continue into the future. Any visual difference in the pages should be handled by the tiptap rendering. So we should be able to get rid of both ArticleDetailPage and NewsDetailPage in favor of WebsiteContentDetail:
app-pages/articles/ ArticleListingPage.tsx app-pages/news/ NewsListingPage.tsx app-pages/website_content/ WebsiteContentDetail.tsx (suggested as new, to replace ArticleDetailPage, etc) WebsiteContentDraftListingPage.ts (exists) WebsiteContentEditPage.tsx (exists) WebsiteContentNewPage.tsx (exists) + test filesIMO Makes sense to keep the two separate route directories
app/news/...andapp/articles..., though. -
Dead Code:
ArticleNewPage,ArticleEditPage,UserArticleEditPage,UserArticleNewPage... aren't these all dead code now thatWebsiteContentEditPageetc exist?⚠️ ArticleNewPage.test.tsx(existing) should probably be converted intoWebsiteContentNewPage.test.tsxbefore deleting the former.
-
urls.ts: This includes things like
ARTICLES_LISTING=/news. Could you double check this file? -
TiptapEditor/index.ts:2exportsArticleEditor as UserArticleEditorbut nothing uses this. Let's delete the export. -
Styling Comment: Copilot's comment #3344 (comment) is accurate and worth fixing; besides the fragile selector, should replace
abcwith a useful class name. -
⚠️ ⚠️ Can't create articles: When try creating a new article at http://open.odl.local:8062/website_content/article/new, thecontent_typeis"news". -
Listing Page Filter: See #3344 (comment)
-
byLineViewer type: See #3344 (comment)
-
Remove Dead Props: ArticleEditorProps in
TiptapEditor/contentTypes/article/ArticleEditor.tsxmarks several things as deprecated, but they are never used. Just delete them. Same for/contentTypes/news/NewsEditor.tsx -
Remove Deprecated ArticleByLineInBannerViewer: At https://github.com/mitodl/mit-learn/pull/3344/changes#diff-189cba8943939cc1aa7937abb3d5f8f9e592f1777749d828f954f2abe05e0c04R43 There's only one callsite, just remove the deprecated thing.
-
Permission.ArticleEditor: Either rename this and update the backend serializer, or if you want to keep backend untouched for this PR, at least leave a comment that the permission is actually for all website_content; I'd be ok with teither for now.
Note
Would suggest doing a few copilot local reviews making sure everything is addressed... a few of Copilot's relevant comments were not addressed, so a local copilot review helps make sure.
ahtesham-quraish
force-pushed
the
ahtesham/tiptap-front-branch
branch
from
8188ed6 to
454936e
Compare
ahtesham-quraish
added
Needs Review
I don't think these got addressed. I had copilot run a quick review locally, and it caught some more things:
|
ahtesham-quraish
force-pushed
the
ahtesham/tiptap-front-branch
branch
3 times, most recently
from
f95700b to
ea23f51
Compare
ahtesham-quraish
force-pushed
the
ahtesham/tiptap-front-branch
branch
2 times, most recently
from
405ed1e to
bc6a4ac
Compare
ahtesham-quraish
force-pushed
the
ahtesham/tiptap-front-branch
branch
3 times, most recently
from
5fab43c to
c49de33
Compare
ahtesham-quraish
force-pushed
the
ahtesham/tiptap-front-branch
branch
from
1c9e448 to
fe59476
Compare
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
This is looking much cleaner. Some comments below, including one styling regression for published news articles.
There is also some complication around the banner / byline difference sbetween news + article that I hadn't appreciated, and which I'll comment about in a followup.
- http://open.odl.local:8062/website_content/drafts/ with no query param seems to show "Articles" title but display all website content.
- I think the simplest thing for now would be to make the default behave like content_type=news. What do you think?
- A few lingering renames
- WebsiteContentContext still has its field named article?: WebsiteContent, and the core WebsiteContentEditor prop is still article
(NewsEditor renamed its own prop but passes article={newsItem}). - Some lingering UserArticle references (in comments)
- WebsiteContentDetail uses
articleIdwhere it should usecontentId. Some internal article naming to WebsiteContentDetail, see https://github.com/mitodl/mit-learn/pull/3344/changes#r3289665347
- WebsiteContentContext still has its field named article?: WebsiteContent, and the core WebsiteContentEditor prop is still article
/articles/should be read-only, like "news", right?⚠️ Per discussion in slack, I lean toward just getting rid of this route for now, until we get designs from Bilal.
- https://github.com/mitodl/mit-learn/pull/3344/changes#r3289681768
- Metadata... Let's share extractImageMetadata + add a content_type check https://github.com/mitodl/mit-learn/pull/3344/changes#r3289724607
⚠️ Styling regression vs main, see https://github.com/mitodl/mit-learn/pull/3344/changes#r3290090895- Banner breadcrumb text, see https://github.com/mitodl/mit-learn/pull/3344/changes#r3290389595
- Please see #3344 (comment).
New Routes
The new routes make sense to me, but I think we should (A) remove /articles/new, /articles/draft, and /articles/[slugOrId]/edit and (B) double check with Ferdi if we actually want /articles/ route at all yet / if it should be feature flagged.
/website_content/[type]/new
/website_content/[type]/[idOrSlug]/edit
/website_content/drafts?content_type=...
NEW ARTICLE ROUTES:
PUBLIC
/articles <-- Let's check if we want this
/articles/[slugOrId]
PRIVATE:
/articles/[slugOrId]/draft
/articles/new <-- suggest removing
/articles/draft <-- suggest removing
/articles/[slugOrId]/edit <-- suggest removing
EXISTING NEWS ROUTES
PUBLIC
/news
/news/[slugOrId]
PRIVATE
/news/[slugOrId]/draft
/news/new <-- unnecessary, but redirects for backward compat
/news/draft <-- unnecessary, but redirects for backward compat
/news/[slugOrId]/edit <-- unnecessary, but redirects for backward compat
| learningResourceIds?: number[] | ||
| }) => { | ||
| const { data: article, isLoading } = useArticleDetailRetrieve(articleId) | ||
| const { data: article, isLoading } = |
There was a problem hiding this comment.
This should be content, right? Not article. Same for the prop articleId above... should be contentId.
| {article.content_type === "article" ? ( | ||
| <ArticleEditor article={article} readOnly /> | ||
| ) : ( | ||
| <NewsEditor newsItem={article} readOnly /> | ||
| )} |
There was a problem hiding this comment.
We will be adding more content types, so a dictionary lookup would be more scalable here than conditionals.
Alternatively, rename WebsiteContentEditor to BaseWebsiteContentEditor and have WebsiteContentEditor itself switch based on content_type.
| ) | ||
|
|
||
| const description = extractArticleDescription(article) | ||
| const leadImage = extractImageMetadata(article) |
There was a problem hiding this comment.
extractImageMetadata is copied between both article/ and /news pages. I think we should put this in @/common/website_content.ts. You could do the same for extractArticleDescription, though that more conceivably differs between content types (even though it does not right now).
Additionally, I'd suggest renaming article to content, and adding a check
if (content.content_type != "article") notFound()and a similar check on the news page.
| const toolbarSlot = readOnly ? ( | ||
| <> | ||
| <Spacer /> | ||
| <ButtonLink | ||
| variant="secondary" | ||
| href="/website_content/drafts?content_type=article" | ||
| size="small" | ||
| > | ||
| Drafts | ||
| </ButtonLink> | ||
| <ButtonLink variant="primary" href={editUrl} size="small"> | ||
| Edit | ||
| </ButtonLink> | ||
| </> | ||
| ) : null |
There was a problem hiding this comment.
toolbarSlot is a function purely of content_type.
If you give WebsiteContentEditor a content_type: WebsiteContentContentTypeEnum prop, you could construct it there very easily rather than having both ArticleEditor and NewsEditor construct it.
- "Drafts" link →
websiteContentDraftsView(contentType) - "Edit" link →
websiteContentEditView(contentType, slugOrId)
| "&& .tiptap.ProseMirror.tiptap-viewer > :nth-child(2)": { | ||
| paddingTop: "36px", | ||
| }, |
There was a problem hiding this comment.
IMO:
- if we actually need this, it should be scoped to articles only.
- But maybe we can just remove it ? (Do we have designs for this page?)
| variant="dark" | ||
| ancestors={[ | ||
| { href: "/", label: "Home" }, | ||
| { href: "/articles", label: "MIT Learn Articles" }, |
There was a problem hiding this comment.
Just saw discussion in slack... This should be just "Articles" not "MIT Learn Articles"
|
@ahtesham-quraish I took a closer look at the banner and byline nodes today. Details below, along with an example commit I think you should apply to this branch. (But please read this and the commit carefully to see if you agree.) The byline is more different than it needs to be between article and news: News has a single render path for the byline, article has two, which leads to some bugs. More broadly, the article editor has differences between read-only and editing mode that it should not have. Here's a more detailed summary:
This leads to divergence between the editing and read-only views for articles. Concretely:
We should unify the two views:
I took this approach in 35f6bf9 (branch StylingYou'll see now "news" and "article" use exactly the same ByLineInfoBar node. They are just styled slightly differently via a classname on WebsiteContentEditor. In general, where UI differs between article vs news vs other content types but is structurally the same (author - reading time - date - share) we should account for the differences just with CSS. (Aside: I would say that if they are structurally very similar, but slightly different, then we should at least raise with designers (Bilal) whether they can be made structurally identical.) Additionally, you'll see in the commit above that all styling differences for ByLine between article and news live in ArticleEditor.tsx. This is better than putting them at the node level...As we add more content types, individual nodes should not need to know about the content type, at least if the differences are style. Note: I also dropped the backgroundColor prop on WebsiteContentEditor in the commit above. No need for a prop, can just be css via styled(...). |
What are the relevant tickets?
https://github.com/mitodl/hq/issues/11232
Description (What does it do?)
Step 2: Frontend editor refactor (Second PR)
Make
page-components/TiptapEditor/content-type-agnostic. Success looks like:/newspages./articlesthat mirrors/newsfor now, with minimal themeing changes. More changes to come, but start similar.Screenshots (if appropriate):
How can this be tested?
For testing you need to first take pull of this branch and then run the containers like web and celery and frontend, then you need to visit the following url
http://open.odl.local:8062/news/new, you first create the article and check if the request is being made tohttp://api.open.odl.local:8065/api/v1/website_content/url, basically, you need to test every corner of the articles CRUD and verify everything looks good.You should keep in mind that when article is created then celery task is run which sync the article to news, so It should run to and you will be able to see the all the articles using the following urls
http://open.odl.local:8062/newshttp://open.odl.local:8062/website_content/article/newAdditional Context