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

refactor: use frontend-component-header, move away from old studio styles #74

Merged

Conversation

brian-smith-tcril
Copy link
Contributor

this PR has all the changes from #69

the differences are:

  • the commits are squashed (the WIP pr has a lot of unhelpful commit messages, as well as commits that aren't at working stages)
  • the package.json points to the frontend-component-header from npm instead of from my fork

@brian-smith-tcril brian-smith-tcril force-pushed the mergable-header-component-usage branch 19 times, most recently from 3754d8a to f7084fb Compare October 7, 2022 15:23
@codecov
Copy link

codecov bot commented Oct 7, 2022

Codecov Report

Base: 49.07% // Head: 48.61% // Decreases project coverage by -0.45% ⚠️

Coverage data is based on head (8176c31) compared to base (a640562).
Patch coverage: 54.54% of modified lines in pull request are covered.

❗ Current head 8176c31 differs from pull request most recent head 2814965. Consider uploading reports for the commit 2814965 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #74      +/-   ##
==========================================
- Coverage   49.07%   48.61%   -0.46%     
==========================================
  Files          76       76              
  Lines        1997     1989       -8     
  Branches      373      367       -6     
==========================================
- Hits          980      967      -13     
- Misses        983      988       +5     
  Partials       34       34              
Impacted Files Coverage Δ
src/index.jsx 0.00% <ø> (ø)
src/library-authoring/author-library/messages.js 100.00% <ø> (ø)
src/library-authoring/common/LicenseField.jsx 25.00% <0.00%> (ø)
...thoring/configure-library/LibraryConfigurePage.jsx 3.44% <0.00%> (ø)
...y-authoring/course-import/CourseImportListItem.jsx 92.30% <ø> (ø)
.../library-authoring/edit-block/LibraryBlockPage.jsx 3.15% <0.00%> (+0.03%) ⬆️
...ary-authoring/library-access/LibraryAccessPage.jsx 11.32% <0.00%> (+0.60%) ⬆️
...ibrary-authoring/studio-header-wrapper/messages.js 100.00% <ø> (ø)
...ring/studio-header-wrapper/StudioHeaderWrapper.jsx 30.00% <30.00%> (ø)
...horing/studio-header-wrapper/ContentTitleBlock.jsx 50.00% <50.00%> (ø)
... and 7 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

it('Fetches block LTI URL to clipboard', async () => {
const library = libraryFactory({ allow_lti: true });
const blocks = makeN(blockFactoryLine([], { library }), 2);
// todo: figure out how LTI stuff works
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@arbrandes i'm not sure how to test this. i'll gladly bring back the buttons and uncomment these tests once i know how to set up a library to have LTI stuff

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's push the LTI stuff back until the rest is done - maybe even further. It requires backend fixes before we can revisit the frontend properly.

@brian-smith-tcril brian-smith-tcril force-pushed the mergable-header-component-usage branch 2 times, most recently from f7084fb to b788d97 Compare October 11, 2022 15:08
@arbrandes arbrandes linked an issue Oct 12, 2022 that may be closed by this pull request
@arbrandes
Copy link
Contributor

@brian-smith-tcril, tested it, and it (still) works great! Can you please rebase on master?

Also, it would be great if we didn't have to merge with a failing codecov/project. I know, it's just a -0.46% diff at time of writing, but it wouldn't be a great example to set. (There's a separate issue in the spring cleaning checklist for tests, so we could think of this as a start-off point.)

@brian-smith-tcril
Copy link
Contributor Author

@arbrandes this is ready for eyes again

i updated the header PR and created a testing branch that updates the package.json to point to a branch of the forked header with dist checked in

Copy link
Contributor

@arbrandes arbrandes left a comment

Choose a reason for hiding this comment

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

@brian-smith-tcril, merged the frontend-component-header PR. Tested it in the process of testing this (via your test branch). Looks great! Let me know if we can merge it as is, or if we need a version bump for frontend-component-header.

@brian-smith-tcril
Copy link
Contributor Author

@arbrandes a version bump on the header repo would be ideal. just for peace of mind i'd like to verify everything is working in the code we're about to merge before hitting the button

@arbrandes arbrandes merged commit 1f4e714 into openedx:master Oct 21, 2022
@brian-smith-tcril brian-smith-tcril deleted the mergable-header-component-usage branch October 21, 2022 13:05
@arbrandes arbrandes linked an issue Oct 31, 2022 that may be closed by this pull request
4 tasks
@arbrandes arbrandes removed a link to an issue Oct 31, 2022
4 tasks
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.

Use a shared header component, instead of a custom one.
2 participants