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

Refactor copy function into its own folder + fix blog typo #629

Merged
merged 20 commits into from Oct 29, 2022
Merged

Refactor copy function into its own folder + fix blog typo #629

merged 20 commits into from Oct 29, 2022

Conversation

anhduy1202
Copy link
Collaborator

Description

  • I've refactored the copy function into its own folder lib/public/copy according to @EthanThatOneKid suggestion
  • I've removed the gibberish typo on the blog

Result

The copy feature works fine when I tested

@vercel
Copy link

vercel bot commented Oct 23, 2022

Someone is attempting to deploy a commit to a Personal Account owned by @EthanThatOneKid on Vercel.

@EthanThatOneKid first needs to authorize it.

@anhduy1202 anhduy1202 linked an issue Oct 23, 2022 that may be closed by this pull request
Blog: Regression introduced in copy code feature #628
Closed
jaasonw
jaasonw suggested changes Oct 23, 2022
View reviewed changes
Copy link
Contributor

@jaasonw jaasonw left a comment

Choose a reason for hiding this comment

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

cant provide a more comprehensive review rn but

  1. toast is missing border
  2. copy doesnt work on events anymore
  3. document the copy function

@anhduy1202
Copy link
Collaborator Author

cant provide a more comprehensive review rn but

  1. toast is missing border
  2. copy doesnt work on events anymore
  3. document the copy function

Screen Shot 2022-10-22 at 8 34 56 PM

EthanThatOneKid
EthanThatOneKid requested changes Oct 23, 2022
View reviewed changes
Copy link
Owner

@EthanThatOneKid EthanThatOneKid left a comment •
edited

Choose a reason for hiding this comment

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

I did not review the techniques used in #621 before it merged, so I left some comments on code written in that PR. Thank you for following up with this PR!

src/lib/public/copy/copy.ts Outdated Show resolved Hide resolved
src/lib/public/copy/copy.ts Outdated Show resolved Hide resolved
src/routes/(site)/blog/[id]/+page.svelte Outdated Show resolved Hide resolved
src/routes/(site)/blog/[id]/blog-body.svelte Outdated Show resolved Hide resolved
src/routes/(site)/blog/[id]/blog-body.svelte Outdated Show resolved Hide resolved
EthanThatOneKid
EthanThatOneKid approved these changes Oct 23, 2022
View reviewed changes
Copy link
Owner

@EthanThatOneKid EthanThatOneKid left a comment

Choose a reason for hiding this comment

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

Thank you for modularizing and refactoring the copy function! This functionality was used in several places on the site, so it was a perfect candidate for such a refactor. Looks great!

@vercel
Copy link

vercel bot commented Oct 23, 2022 •
edited

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
acm-csuf-site ✅ Ready (Inspect) Visit Preview Oct 29, 2022 at 9:24PM (UTC)

@vercel vercel bot temporarily deployed to Preview October 23, 2022 21:26 Inactive
jaasonw
jaasonw suggested changes Oct 23, 2022
View reviewed changes
Copy link
Contributor

@jaasonw jaasonw left a comment

Choose a reason for hiding this comment

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

also border is not work on blog notif

image

src/routes/(site)/blog/[id]/blog-body.svelte Show resolved Hide resolved
src/lib/public/copy/copy.ts Show resolved Hide resolved
jaasonw
jaasonw suggested changes Oct 23, 2022
View reviewed changes
src/routes/(site)/events/event.svelte Outdated Show resolved Hide resolved
@anhduy1202
Copy link
Collaborator Author

Update

  • Toast pop-up border color now fixed ( I used acm-general color for the blog popup )
  • Copy icon now change color depends on dark/light mode ( I can refactor the MutationObserver function into a new file if you want )

Screenshots

Screen Shot 2022-10-28 at 12 30 06 PM

Screen Shot 2022-10-28 at 12 30 44 PM

Remaining Issues:

Screen Shot 2022-10-28 at 12 32 43 PM

EthanThatOneKid
EthanThatOneKid approved these changes Oct 29, 2022
View reviewed changes
Copy link
Owner

@EthanThatOneKid EthanThatOneKid left a comment

Choose a reason for hiding this comment

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

LGTM!

@EthanThatOneKid EthanThatOneKid enabled auto-merge (squash) October 29, 2022 19:46
@vercel vercel bot temporarily deployed to Preview October 29, 2022 19:47 Inactive
@vercel vercel bot temporarily deployed to Preview October 29, 2022 21:24 Inactive
jaasonw
jaasonw suggested changes Oct 29, 2022
View reviewed changes
src/routes/(site)/blog/[id]/+page.svelte Outdated Show resolved Hide resolved
src/routes/(site)/blog/[id]/blog-body.svelte Outdated Show resolved Hide resolved
@EthanThatOneKid EthanThatOneKid merged commit 2912a41 into EthanThatOneKid:main Oct 29, 2022
@EthanThatOneKid EthanThatOneKid mentioned this pull request Oct 31, 2022
Merged
EthanThatOneKid added a commit that referenced this pull request Oct 31, 2022
@EthanThatOneKid
Roll forward #629 (#648)
c13ab41 
> PR #629 introduced a regression by ignoring the changes made in #630. This PR reintroduces the changes from #630.
@EthanThatOneKid EthanThatOneKid mentioned this pull request Nov 6, 2022
Merged
@karnikaavelumani karnikaavelumani mentioned this pull request Nov 7, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaasonw jaasonw jaasonw approved these changes

@EthanThatOneKid EthanThatOneKid EthanThatOneKid approved these changes

@karnikaavelumani karnikaavelumani Awaiting requested review from karnikaavelumani

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Blog: Regression introduced in copy code feature
3 participants
@anhduy1202 @jaasonw @EthanThatOneKid