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.

Sign up for GitHub

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

Jump to bottom

build: edx namespace packages upgrade & resolved respective eslint issue #508

Merged
merged 6 commits into from
May 24, 2023
Merged

build: edx namespace packages upgrade & resolved respective eslint issue #508

merged 6 commits into from
May 24, 2023

Conversation

BilalQamar95
Copy link
Contributor

Description

This PR upgrades @edx/frontend-build along with @edx/frontend-component-footer, @edx/frontend-component-header, @edx/frontend-platform packages & resolves respective eslint issues.

How Has This Been Tested?

Please describe in detail how you tested your changes.

Screenshots/sandbox (optional):

Include a link to the sandbox for design changes or screenshot for before and after. Remove this section if it's not applicable.

Before After

Merge Checklist

  • If your update includes visual changes, have they been reviewed by a designer? Send them a link to the Sandbox, if applicable.
  • Is there adequate test coverage for your changes?

Post-merge Checklist

  • Deploy the changes to prod after verifying on stage or ask @openedx/edx-infinity to do it.
  • 🎉 🙌 Celebrate! Thanks for your contribution.

@BilalQamar95 BilalQamar95 self-assigned this May 3, 2023
@codecov
Copy link

codecov bot commented May 3, 2023 •
edited
Loading

Codecov Report

Patch coverage: 87.83% and project coverage change: +0.13 🎉

Comparison is base (8228549) 91.74% compared to head (4f75a10) 91.88%.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master     #508      +/-   ##
==========================================
+ Coverage   91.74%   91.88%   +0.13%     
==========================================
  Files         170      170              
  Lines        3464     3474      +10     
  Branches      900      905       +5     
==========================================
+ Hits         3178     3192      +14     
+ Misses        267      263       -4     
  Partials       19       19
Impacted Files Coverage Δ
src/components/icons/InsertLink.jsx 100.00% <ø> (ø)
src/components/icons/Issue.jsx 100.00% <ø> (ø)
src/components/icons/People.jsx 0.00% <ø> (ø)
src/components/icons/PushPin.jsx 100.00% <ø> (ø)
src/components/icons/Question.jsx 100.00% <ø> (ø)
src/components/icons/QuestionAnswer.jsx 0.00% <ø> (ø)
src/components/icons/QuestionAnswerOutline.jsx 100.00% <ø> (ø)
src/components/icons/StarFilled.jsx 100.00% <ø> (ø)
src/components/icons/StarOutline.jsx 100.00% <ø> (ø)
src/components/icons/ThumbUpFilled.jsx 100.00% <ø> (ø)
... and 29 more

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

@BilalQamar95 BilalQamar95 force-pushed the bilalqamar95/packages-upgrade branch from c1b5a44 to f46d3f4 Compare May 4, 2023 09:27
@BilalQamar95 BilalQamar95 changed the title Package upgrades build: edx namespace packages upgrade & resolved respective eslint issue May 4, 2023
OmarIthawi
OmarIthawi approved these changes May 11, 2023
View reviewed changes
package.json Show resolved Hide resolved
awais-ansari
awais-ansari reviewed May 22, 2023
View reviewed changes
Copy link
Contributor

@awais-ansari awais-ansari left a comment

Choose a reason for hiding this comment

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

@BilalQamar95 Please resolve conflicts. The CI pipeline is unblocked now. We can merge this PR after proper testing. Thanks.

@BilalQamar95 BilalQamar95 mentioned this pull request May 22, 2023
Merged
@BilalQamar95 BilalQamar95 force-pushed the bilalqamar95/packages-upgrade branch from d08bffa to 21f55a9 Compare May 23, 2023 08:26
@BilalQamar95
Copy link
Contributor Author

@BilalQamar95 Please resolve conflicts. The CI pipeline is unblocked now. We can merge this PR after proper testing. Thanks.

@awais-ansari I have resolved the conflicts, PR is now ready for review.

awais-ansari
awais-ansari requested changes May 23, 2023
View reviewed changes
Copy link
Contributor

@awais-ansari awais-ansari left a comment

Choose a reason for hiding this comment

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

@BilalQamar95 Just a minor change request other than LGTM.

package.json Outdated Show resolved Hide resolved
muhammadadeeltajamul
muhammadadeeltajamul reviewed May 23, 2023
View reviewed changes
Copy link
Contributor

@muhammadadeeltajamul muhammadadeeltajamul left a comment

Choose a reason for hiding this comment

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

Some minor nits. Rest looks good.

Can we change function definitions to
'const functionName = async() => {}' in the following files

src/discussions/post-comments/data/api.js
src/discussions/posts/data/api.js

@BilalQamar95
Copy link
Contributor Author

BilalQamar95 commented May 23, 2023 •
edited
Loading

Some minor nits. Rest looks good.

Can we change function definitions to 'const functionName = async() => {}' in the following files

src/discussions/post-comments/data/api.js src/discussions/posts/data/api.js

@muhammadadeeltajamul Updated PR with suggested changes

@abdullahwaheed
Copy link
Contributor

@awais-ansari could you please review and merge this PR

@awais-ansari awais-ansari merged commit 70f6541 into master May 24, 2023
@awais-ansari awais-ansari deleted the bilalqamar95/packages-upgrade branch May 24, 2023 06:55
snglth pushed a commit to Abstract-Tech/community-theme-discussions that referenced this pull request Jan 9, 2024
@BilalQamar95 @snglth
build: edx namespace packages upgrade & resolved respective eslint is…
0230354 
…sue (openedx#508)

* refactor: updated frontend-build, frontend-platform, header & footer packages

* fix: resolved eslint issues post frontend-build upgrade

* refactor: resolved eslint issues

* refactor: pinned frontend-build & changed suggested function definitions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OmarIthawi OmarIthawi OmarIthawi approved these changes

@awais-ansari awais-ansari awais-ansari approved these changes

@muhammadadeeltajamul muhammadadeeltajamul muhammadadeeltajamul approved these changes

@sundasnoreen12 sundasnoreen12 Awaiting requested review from sundasnoreen12

Assignees

@BilalQamar95 BilalQamar95

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@BilalQamar95 @abdullahwaheed @OmarIthawi @muhammadadeeltajamul @awais-ansari