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

rename suffixes from .mdx to .md #2353

Closed
wants to merge 3 commits into from

Conversation

Madoshakalaka
Copy link
Contributor

GitLocalize can't recognize .mdx suffixes.

What's wrong with you GitLocalize??

@github-actions
Copy link

github-actions bot commented Jan 10, 2022

Visit the preview URL for this PR (updated for commit 9c3f7ca):

https://yew-rs--pr2353-docs-mdx-to-md-1xzyvthn.web.app

(expires Wed, 19 Jan 2022 15:02:27 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@Madoshakalaka
Copy link
Contributor Author

@voidpumpkin
Copy link
Member

voidpumpkin commented Jan 10, 2022

@futursolo https://github.com/yewstack/yew/runs/4757255525?check_suite_focus=true#step:8:224 do you know why it fails here?

SuspensionHandle does not implement Copy and move occurs because resume method takes self not &self.
you need to clone before calling resume
handle.clone().resume()

@futursolo
Copy link
Member

@futursolo https://github.com/yewstack/yew/runs/4757255525?check_suite_focus=true#step:8:224 do you know why it fails here?

SuspensionHandle does not implement Copy and move occurs because resume method takes self not &self. you need to clone before calling resume handle.clone().resume()

SuspensionHandle does not implement Clone either.

*.md was never run under website-test.

Please see the fix here:

https://github.com/yewstack/yew/pull/2335/files#diff-624ff249422d648833239e5ff80a42fd8b626084e71042c757f4ae59d9117c16R95

voidpumpkin
voidpumpkin previously approved these changes Jan 12, 2022
Copy link
Member

@voidpumpkin voidpumpkin left a comment

Choose a reason for hiding this comment

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

Looks good

@voidpumpkin voidpumpkin requested a review from ranile January 12, 2022 11:31
@ranile
Copy link
Member

ranile commented Jan 12, 2022

@Madoshakalaka please rebase the changes from master. Looks good otherwise

# Conflicts:
#	tools/website-test/build.rs
#	website/README.md
@Madoshakalaka
Copy link
Contributor Author

hmmm I don't think I understand the mechanisms of merge conflicts fully...but anyways, the PR branch does have the correct suspense.md file. This is OK to merge.

@voidpumpkin
Copy link
Member

voidpumpkin commented Jan 18, 2022

hmmm I don't think I understand the mechanisms of merge conflicts fully...but anyways, the PR branch does have the correct suspense.md file. This is OK to merge.

it has correct file extension because your branch is not rebased onto latest master branch.
I would guess there will be 2 suspense files if we merge. Otherwise indeed wtf

Copy link
Member

@voidpumpkin voidpumpkin left a comment

Choose a reason for hiding this comment

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

to avoid github conflicts magic, i will only approve once this is rebased.

@voidpumpkin
Copy link
Member

hmmm I don't think I understand the mechanisms of merge conflicts fully...but anyways, the PR branch does have the correct suspense.md file. This is OK to merge.

it has correct file extension because your branch is not rebased onto latest master branch. I would guess there will be 2 suspense files if we merge. Otherwise indeed wtf

nvm i pieced it together.
This PR did not modify suspense file thus no conflict. If we merge without rebasing we will simply have a suspense.mdx file in repo.
When you checked if suspense had correct file extention did you rebase onto latest master into your local repo?

@Madoshakalaka
Copy link
Contributor Author

closed because GitLocalize now has MDX support! See gitlocalize/feedback#112

@Madoshakalaka Madoshakalaka deleted the docs/mdx-to-md branch January 26, 2022 06:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants