-
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
Answers Tiny MCE tutorial #1
base: master
Are you sure you want to change the base?
Conversation
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.
Really nice work on this Ben! 🏆
I've largely just left a few style comments to help when you get around to making changes elsewhere, there were 2 things that if you have time though it'd be good to have another look at.
src/main/html/Part1Ex1.html
Outdated
tinymce.init({ selector: '#editor2' }); | ||
tinymce.init({ selector: '#editor3' }); |
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.
This could just be simplified to this for what it's worth 🤷
tinymce.init({ selector: '#editor2' }); | |
tinymce.init({ selector: '#editor3' }); | |
tinymce.init({ selector: '.editorSet' }); |
src/main/ts/Part2Ex1.ts
Outdated
@@ -16,6 +16,10 @@ Let's model the x,y of the top-left and bottom-right corners. | |||
*/ | |||
export interface Boundz { | |||
// TODO: add fields: x1, y1, x2, y2 | |||
x1: number; |
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.
For this it's fine, but just as a heads up you'll see we normally mark most properties readonly
since that helps to ensure we're not mutating data.
@@ -41,6 +41,7 @@ export const myFrogs: Frog[] = [ | |||
|
|||
export const runEach2 = (): void => { | |||
// TODO: Use Arr.each and console.log to print the name of each frog | |||
Arr.each(myFrogs, (f: Frog) => console.log(f.name)) |
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.
Arr.each(myFrogs, (f: Frog) => console.log(f.name)) | |
Arr.each(myFrogs, (f) => console.log(f.name)) |
You don't need to declare the type here as it'll be inferred. We normally try to avoid re-declaring redundant types as if you have to change it later it just makes more work 🤷
@@ -83,12 +83,10 @@ export const evens = (xs: number[]): number[] => | |||
|
|||
// TODO: Write a function that returns all the frogs that ribbit | |||
// TODO: Run the provided test to check your answer. | |||
export const ribbitting = (frogs: Frog[]): Frog[] => | |||
[]; | |||
export const ribbitting = (frogs: Frog[]): Frog[] => Arr.filter(frogs, (f: Frog) => f.ribbits === true) |
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.
export const ribbitting = (frogs: Frog[]): Frog[] => Arr.filter(frogs, (f: Frog) => f.ribbits === true) | |
export const ribbitting = (frogs: Frog[]): Frog[] => Arr.filter(frogs, (f: Frog) => f.ribbits) |
The === true
here is redundant since it's a non-optional boolean, so we're normally just do something like the above instead.
src/main/ts/Part2Ex3Optional.ts
Outdated
@@ -1,4 +1,5 @@ | |||
import { Optional } from '@ephox/katamari'; | |||
import { search } from '@ephox/sugar/lib/main/ts/ephox/sugar/api/dom/Focus'; |
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.
Something went wrong with an auto import here I'm guessing... you'll find you'll get a lint error in any of our regular code for this fwiw. We also prefer used named imports as it makes the code more readable, e.g:
import { Focus } from `@ephox/katamari';`
const switchMode = (m: Mode): void => { | ||
const valid = hasMode(m); |
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.
This doesn't really do anything meaningful unless you're accepting content from outside of TypeScript, as you can't pass a mode that isn't defined in the Mode
type and if you're checking this it's somewhat defeating the point of using types. That said, you've still made a pure function here so its fine as that was the point of this exercise.
src/main/ts/Part2Ex4FP.ts
Outdated
|
||
// TODO: Write a function that takes an array of numbers and replaces each value with 9. | ||
export const replaceElementWith9 = (items: number[]) => Optional.from(items).fold(Fun.constant([]), (items) => items.map(Fun.constant(9))); |
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.
This is overly complicated, as again items
shouldn't be nullable so you don't need the Optional.from
logic here. Additionally as a heads up we generally avoid the native Array.map
function and instead use the helper functions defined in Arr
. So instead this could have been something like this:
export const replaceElementWith9 = (items: number[]) => Optional.from(items).fold(Fun.constant([]), (items) => items.map(Fun.constant(9))); | |
export const replaceElementWith9 = (items: number[]) => Arr.map(items, Fun.constant(9)); |
@@ -1,5 +1,7 @@ | |||
import { describe, it } from '@ephox/bedrock-client'; | |||
import { assert } from 'chai'; | |||
import { Optional } from '@ephox/katamari'; | |||
import { createElement } from '@ephox/sugar/lib/main/ts/ephox/sugar/api/node/SugarShadowDom'; |
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.
Another case where we should be imported the named export instead:
import { createElement } from '@ephox/sugar/lib/main/ts/ephox/sugar/api/node/SugarShadowDom'; | |
import { SugarShadowDom } from '@ephox/sugar'; |
src/test/ts/part2/Exercise4FPTest.ts
Outdated
import * as Ex from '../../../main/ts/Part2Ex4FP'; | ||
|
||
describe('Exercise 4 FP', () => { | ||
it('map identity over array', () => { |
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.
Slight style issue here by the looks, as this should be using 2 spaces not 4.
src/test/ts/part3/Part3Ex2Test.ts
Outdated
@@ -112,5 +114,6 @@ describe('Part3Ex3Test', () => { | |||
].join('\n')); | |||
|
|||
// TODO: Write an assertion to test your changes (hint: TinyAssertions) | |||
TinyAssertions.assertContentPresence(editor, {span: 1}); |
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.
This doesn't check the content has been underlined, so it's not a very reliable test as it could just be a span without any styles. Can you take another shot at this please?
@lnewson , can you please give this another review? It can wait till next week since today is Friday |
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.
The fixes looks good thanks Ben 👍
No description provided.