-
Notifications
You must be signed in to change notification settings - Fork 2
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
Cc/smaller bundle #3725
Cc/smaller bundle #3725
Changes from 8 commits
909d589
9b60b67
084f375
131c1e2
af10fed
e54e2cd
9cd41ed
0ea64d0
d417f2a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,7 +1,8 @@ | ||||||
import { faker } from "@faker-js/faker" | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Aside for other mitodl folks: Shinigami92 is one of the faker-js maintainers and previously noted that using specific imports like this would reduce faker's footprint. Personally, I'm somewhat inclined to keep the faker imports as is because:
Shinigami92: If I've misunderstood and there is some benefit I'm not seeing, feel free to let me know. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No you are right 👍 This would only be a benefit until we have managed better locale handling on our side. |
||||||
import * as R from "ramda" | ||||||
import { makePaginatedFactory, Factory } from "ol-util" | ||||||
import { LearningResourceType, factories } from "ol-search-ui" | ||||||
import { makePaginatedFactory, Factory } from "ol-util/build/factories" | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would very much have preferred an import from But as far as I can tell, making that work with Typescript means moving to ESM-style module resolution, which is a fairly big change. So I went for the uglier
Related: https://www.typescriptlang.org/docs/handbook/esm-node.html |
||||||
import { LearningResourceType } from "ol-search-ui" | ||||||
import * as factories from "ol-search-ui/build/factories" | ||||||
import { times } from "lodash" | ||||||
import type { FieldChannel, UserList, UserListItem } from "./interfaces" | ||||||
|
||||||
export const makeUserList: Factory<UserList> = overrides => { | ||||||
|
@@ -10,7 +11,7 @@ export const makeUserList: Factory<UserList> = overrides => { | |||||
short_description: faker.lorem.paragraph(), | ||||||
offered_by: [], | ||||||
title: faker.lorem.words(), | ||||||
topics: R.times(() => factories.makeTopic(), 2), | ||||||
topics: times(2, () => factories.makeTopic()), | ||||||
is_favorite: faker.datatype.boolean(), | ||||||
image_src: new URL(faker.internet.url()).toString(), | ||||||
image_description: faker.helpers.arrayElement([ | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,8 +31,8 @@ $avatarSize: 60px; | |
$carouselSpacing: 24px; | ||
.ic-carousel-card { | ||
height:100%; | ||
margin-left: $carouselSpacing/2; | ||
margin-right: $carouselSpacing/2; | ||
margin-left: $carouselSpacing*0.5; | ||
margin-right: $carouselSpacing*0.5; | ||
Comment on lines
+34
to
+35
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is from running (At one point on this branch I had a failing build that I thought might be related to a sass issue, so I was trying to clean up some warnings.) |
||
} | ||
|
||
.ic-carousel { | ||
|
@@ -56,7 +56,7 @@ $carouselSpacing: 24px; | |
Caveat: This is not a good solution if there is content within $carouselSpacing | ||
of the carousel's left edge. But...there's not. | ||
*/ | ||
transform: translateX(-$carouselSpacing/2); | ||
transform: translateX(-$carouselSpacing*0.5); | ||
/* | ||
We also want the carousel contents (cards) to appear as if they are full | ||
width. By default, the width is 100% and since there's cell-spacing, the | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,9 +1,8 @@ | ||||||
//@ts-expect-error casual-browserify does not have typescript types | ||||||
import casual from "casual-browserify" | ||||||
import { faker } from "@faker-js/faker" | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
import R from "ramda" | ||||||
import { DATE_FORMAT } from "./util" | ||||||
import { Factory } from "ol-util" | ||||||
import { Factory } from "ol-util/build/factories" | ||||||
import { | ||||||
CourseTopic, | ||||||
LearningResourceResult, | ||||||
|
@@ -13,7 +12,7 @@ import { | |||||
CardMinimalResource, | ||||||
EmbedlyConfig | ||||||
} from "./interfaces" | ||||||
import { pick } from "lodash" | ||||||
import { pick, times } from "lodash" | ||||||
|
||||||
const OPEN_CONTENT = "Open Content" | ||||||
const PROFESSIONAL = "Professional Offerings" | ||||||
|
@@ -51,7 +50,7 @@ export const makeCourseResult: Factory<LearningResourceResult> = overrides => ({ | |||||
offered_by: [casual.random_element(["edx", "ocw"])], | ||||||
topics: [casual.word, casual.word], | ||||||
object_type: LearningResourceType.Course, | ||||||
runs: R.times(() => makeRun(), 3), | ||||||
runs: times(3, () => makeRun()), | ||||||
is_favorite: casual.coin_flip, | ||||||
lists: casual.random_element([[], [100, 200]]), | ||||||
audience: casual.random_element([ | ||||||
|
@@ -127,7 +126,7 @@ export const makeSearchResponse = ( | |||||
type: string | null = null, | ||||||
withFacets = true | ||||||
) => { | ||||||
const hits = R.times(() => makeSearchResult(type), pageSize) | ||||||
const hits = times(pageSize, () => makeSearchResult(type)) | ||||||
return { | ||||||
hits: { | ||||||
total, | ||||||
|
@@ -175,10 +174,10 @@ export const makeLearningResource: Factory<LearningResource> = overrides => { | |||||
id: faker.unique(faker.datatype.number), | ||||||
title: faker.lorem.words(), | ||||||
image_src: new URL(faker.internet.url()).toString(), | ||||||
topics: R.times(() => makeTopic(), 2), | ||||||
topics: times(2, () => makeTopic()), | ||||||
object_type: makeLearningResourceType(), | ||||||
platform: faker.lorem.word(), | ||||||
runs: R.times(() => makeRun(), 3), | ||||||
runs: times(3, () => makeRun()), | ||||||
lists: [], | ||||||
...overrides | ||||||
} | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,2 @@ | ||
export * from "./components" | ||
export * from "./interfaces" | ||
export * as factories from "./factories" |
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.
I also removed all usage of ramda in this PR because:
If we need it for something, we can definitely add it back.