feat(Codebytes): add tests for codebytes package disc 399 by BandanaKM · Pull Request #21 · Codecademy/client-modules

BandanaKM · 2022-01-22T19:31:37Z

Overview

PR Checklist

Related to JIRA ticket: DISC-399
I have run this code to verify it works
This PR includes unit tests for the code change

Notes:

This pull request migrates tests for the Codebytes component. Please note any changes between the original and the tests in this pull request are described below:

Original tests in static-sites: https://github.com/codecademy-engineering/static-sites/tree/577de863974111b4021c2bec6f4b5d2b1684a3c8/src/templates/CodeByteEditor

See tests directory

codebytes-test.tsx
- switched userEvent in place of fireEvent and changed language select to follow userEvent syntax (examples of this are on line 81 ,93)
- added a describe block for ChangeHandlers. this tests the two callbacks onEdit and onLanguage that accept functions as props. tests ensure these are called with the appropriate data.
- tracking tests are pretty much the same, except trackUserImpression is only called if the isForums prop is true
editor-test.tsx
- again, switched userEvent in place of fireEvent
- again, a describe block for ChangeHandlers. tests ensures onCopy is called with the appropriate data.
language-selection-test.tsx
- identical. just checks for the updated language on the select now.
helpers-test.tsx

removed BBCodeBlock test, since we're not doing that in this helper anymore. this BBCodeBlock helper will be in the next.js/forums version.

Other things:

created a shared files for the mocks that are used both in the codebytes-test and editor-test
codebytes also requires a sibling package (tracking) that also exists in this repo. running yarn --focus works to utilize the sibling package. i needed to add this command in codebytes' package.json, and also run it during the build process in config.yml.

…tests-for-codebytes-package-disc-399

BandanaKM · 2022-01-26T01:41:38Z

+  hideCopyButton?: boolean;
  onCopy?: CodebytesChangeHandler;
-  isIFrame?: boolean;
+  isForums?: boolean;


renaming this prop to just mention that the behavior is really specific to use in forums. figured it's better than 'isIFrame'

If this becomes open source then we will want it to be iFrame again, right? Is there a plan for this to become open source?

yeah we also may want to use codebytes on other non-discourse sites eventually! so +1 using a more generic name

@j10sanders It is opensource as long at the client-modules repo is opensource. I'll change this to a more generic name, maybe isIFrame is better for now and if we decide to ever not use an iFrame in the plugin or on forums we can change!

are you planning on changing the name in this pr?

yes, i may have forgotten to do that here, let me update!

borisonr · 2022-01-26T17:58:44Z

-        .catch(() => console.error('Failed to copy'));
-      onCopy?.(text, language);
+      // eslint-disable-next-line @typescript-eslint/no-floating-promises
+      navigator.clipboard.writeText(text);


why are we removing the catch here?

removed it, because i didn't think it was adding value, just logging an error to the console for the developer. can add it back if we really wish.

the log isnt huge but we probably want to catch an error so it doesnt throw and break the page

absolutely ok, can add back 👍

borisonr · 2022-01-26T18:04:32Z

-  }, []);
+  useEffect(() => {
+    if (language) {
+      setText(initialText ?? (helloWorld[language] || ''));


why are we adding this hook?

From the unit tests in static-sites this intended behavior was made more clear: if the initial text is not provided, then the content is initialized with predefined text for the chosen language. The empty string at the end is for when there is not initial content for that language.

We need it in a useEffect, because if the text and language props change, the change needs to be reapplied.

is there a way for language to change? i don't see a way for someone to do that, in which case there would just be the initial state

@borisonr Hehe, good callout. Since the parent controls the language, and passes it in as props, the language can still technically change, though it's less likely to and more of a design question. This would make the component more flexible, for situations where we may want to pass in a different language from the parent, and what I feel may be be better for opensourcing it.

I can just add it as initial state, though, if we really like, but did not since technically can change.

just trying to be mindful here that this pr was to add tests... probably shouldnt expand scope to potential future features?

but query params would cause a page reload and still work with initial state, no?

if we do want to do with something like initial state, we could do:

const getInitialText = () => { if (initialText !== undefined) return initialText; return initialLanguage ? helloWorld[initialLanguage] : ''; };

and then:

const [text, setText] = useState<string>(getInitialText());

this would also pass the original test

ok with either, using the useEffect for extensibility with props being passed, but could also go with initial state if we want to keep it as state

got it, this works! sorry for the back and forth was just trying to understand the non-test related changes in the pr!

all good! I'll update this pr with this logic and clear out some merge conflicts!

borisonr · 2022-01-31T14:18:12Z

            }
          }}
          snippetsBaseUrl={snippetsBaseUrl}
+          onCopy={onCopy}


did you mean to add oncopy here? was this related to a behavior we didn't realize until we added tests?

@borisonr onCopy is intended to be here! we had it in the original code, and it may have gotten removed with the tracking updates, but we should see it trickle down to the Editor component as we had in previous prs.

when called in the Editor component, we can call it with the specific onCopy behavior that's needed for the iframe version or for the homepage version.

glad we caught it!

@borisonr thank you for your ever-amazing reviews!

codecademydev · 2022-01-31T17:18:01Z

📬Published Alpha Packages:

@codecademy/codebytes@0.5.1-alpha.a075a9.0
@codecademy/styleguide@0.5.1-alpha.a075a9.0

BandanaKM and others added 30 commits December 17, 2021 17:38

working version

489a0c1

update to include gamut styles

f61ddd9

add simple code editor

a90a11a

add editor container styles

db68a21

fix prettier issues

19caf83

pass onCopy from parent

9d702d0

formatting

7cb1098

use isIframeProp

7cd9589

pass down snippets base url from parent

f7fcbbb

simple monaco editor

8efb881

update tslint for scale

f546671

incorporate review feedback

ddee0b1

fix prettier issues

675361b

default snippets endpoint to empty string

263a187

resolve merge conflicts

d70fea1

resolve merge conflicts

a29b647

use rem

d8f87b0

update example text

7386df0

use language prop

01015e9

add language selection comp

094d6b5

use colors gray

33f7ab3

remove env file

47b7061

run prettier

cfe771d

change function name to onEdit

0e05670

update story names

2c28200

comment on change handlers

3da9ab8

update stories

80c1433

fix prettier

9c7a196

pairing session - new props, types, and passing on handler

6d1aaf6

use monaco-editor/react package

f3c23df

BandanaKM added 7 commits January 24, 2022 23:28

call track user impression only if called from forums

b2b8cc2

use isForums prop for tests

f8fe5ce

Merge branch 'hr-bm-migrate-codebytes-tracking-disc-355' into bm-add-…

10e6146

…tests-for-codebytes-package-disc-399

fix identifier

7315cc4

format types

a75010f

try yarn focus

7505e3f

try running codebytes command

be6a662

BandanaKM commented Jan 26, 2022

View reviewed changes

rename to sibling dependencies

9c97400

BandanaKM requested review from a team, dougyd92 and j10sanders and removed request for a team January 26, 2022 03:11

borisonr reviewed Jan 26, 2022

View reviewed changes

Base automatically changed from hr-bm-migrate-codebytes-tracking-disc-355 to main January 27, 2022 14:05

zippyzow requested a review from songstephen as a code owner January 27, 2022 14:05

BandanaKM added 2 commits January 27, 2022 16:35

replace useEffect for initialText with function

7e1da41

resolve merge conflicts

e2bc641

BandanaKM changed the title ~~Bm add tests for codebytes package disc 399~~ Feature(Codebytes) Bm add tests for codebytes package disc 399 Jan 29, 2022

clear merge conflicts

bfc8a83

BandanaKM changed the title ~~Feature(Codebytes) Bm add tests for codebytes package disc 399~~ Feature(Codebytes): add tests for codebytes package disc 399 Jan 30, 2022

BandanaKM changed the title ~~Feature(Codebytes): add tests for codebytes package disc 399~~ feat(Codebytes): add tests for codebytes package disc 399 Jan 30, 2022

BandanaKM requested a review from borisonr January 30, 2022 23:09

borisonr reviewed Jan 31, 2022

View reviewed changes

change to is iframe

a075a9f

borisonr approved these changes Jan 31, 2022

View reviewed changes

BandanaKM merged commit df3f780 into main Jan 31, 2022

BandanaKM deleted the bm-add-tests-for-codebytes-package-disc-399 branch January 31, 2022 18:50

Conversation

BandanaKM commented Jan 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

PR Checklist

Uh oh!

BandanaKM Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

borisonr Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BandanaKM Jan 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BandanaKM Jan 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecademydev commented Jan 31, 2022

📬Published Alpha Packages:

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

BandanaKM commented Jan 22, 2022 •

edited

Loading

BandanaKM Jan 26, 2022 •

edited

Loading

borisonr Jan 26, 2022 •

edited

Loading

BandanaKM Jan 27, 2022 •

edited

Loading

BandanaKM Jan 28, 2022 •

edited

Loading