Skip to content

Fix #791#866

Closed
aenniw wants to merge 1 commit intodocsifyjs:developfrom
aenniw:bug/791
Closed

Fix #791#866
aenniw wants to merge 1 commit intodocsifyjs:developfrom
aenniw:bug/791

Conversation

@aenniw
Copy link
Copy Markdown
Contributor

@aenniw aenniw commented Jun 8, 2019

Signed-off-by: Martin Mihálek [email protected]

Summary

Fixed out of orded embedding of include files

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Docs
  • Build-related changes
  • Other, please describe:

If changing the UI of default theme, please provide the before/after screenshot:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)

You have tested in the following browsers: (Providing a detailed version will be better.)

  • Chrome 74.0.3729.169
  • Firefox
  • Safari
  • Edge
  • IE

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature
  • Related documents have been updated
  • Related tests have been updated

To avoid wasting your time, it's best to open a feature request issue first and wait for approval before working on it.

Other information:

  • DO NOT include files inside lib directory.

@aenniw
Copy link
Copy Markdown
Contributor Author

aenniw commented Jul 6, 2019

Addresses #791

@stale
Copy link
Copy Markdown

stale bot commented Feb 4, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Feb 4, 2020
@anikethsaha anikethsaha requested a review from trusktr February 4, 2020 08:13
@stale stale bot removed the wontfix label Feb 4, 2020
@anikethsaha anikethsaha added the pinned This is to pinned the PR/Issue in order to keep it open label Feb 4, 2020
@anikethsaha
Copy link
Copy Markdown
Member

anikethsaha commented Feb 5, 2020

@aenniw please rebase and resolve the conflicts.
also change the commit message. make it more detailed in order to fit better for changlog

@anikethsaha anikethsaha self-assigned this Feb 5, 2020
@anikethsaha anikethsaha requested a review from a team February 5, 2020 18:37
alexis- added a commit to supermemo-wiki/docsify that referenced this pull request Feb 8, 2020
@alexis-
- Implemented aenniw's fix docsifyjs#866
7291a06
alexis- added a commit to supermemo-wiki/SuperMemoAssistant.Wiki that referenced this pull request Feb 8, 2020
@alexis-
- Temporarily fallback to local docsify (fixed includes being out of …
5f44443 
…order, see docsifyjs/docsify#866)
@aenniw aenniw closed this Feb 23, 2020
@aenniw aenniw deleted the bug/791 branch February 23, 2020 19:22
@aenniw aenniw restored the bug/791 branch February 23, 2020 19:23
@aenniw aenniw reopened this Feb 23, 2020
@anikethsaha
Copy link
Copy Markdown
Member

anikethsaha commented Feb 23, 2020

can you resolve the conflicts

@aenniw
Copy link
Copy Markdown
Contributor Author

aenniw commented Feb 23, 2020

Rebased, however not sure whats Security Flow about...

@anikethsaha anikethsaha reopened this Feb 25, 2020
@randName randName mentioned this pull request Mar 17, 2020
Closed
1 task
trusktr
trusktr requested changes Apr 3, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@trusktr trusktr left a comment

Choose a reason for hiding this comment

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

Thanks for attempting to fix this! Let's make sure

  • current tests pass (I suppose this just requires fixing the conflict)
  • add new tests that ensure multiple embeds work as we're expecting

@aenniw
Copy link
Copy Markdown
Contributor Author

aenniw commented Apr 9, 2020

Not sure how to approach the tests, as It occurs only when embedded files are downloaded out of order mainly due their size difference and network delay...
On local server setup despite big difference in embedded file sizes only way how to partially reproduce the issue is with throttling in browser...

@aenniw
Copy link
Copy Markdown
Contributor Author

aenniw commented Apr 10, 2020

Added e2e scenario and updated live-server to delay serving of one of embedded files to replicate the issue...

anikethsaha
anikethsaha requested changes Apr 10, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@anikethsaha anikethsaha left a comment

Choose a reason for hiding this comment

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

thanks for the update. few notes

@anikethsaha anikethsaha requested review from a team and removed request for a team April 10, 2020 09:26
Koooooo-7
Koooooo-7 reviewed Apr 10, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@Koooooo-7 Koooooo-7 left a comment

Choose a reason for hiding this comment

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

yea, per to the docs, failureThreshold should be changed to smaller.
and I thing It is not a good way that showing test cases on the documentation, it makes users messy.

Koooooo-7
Koooooo-7 reviewed Apr 10, 2020
View reviewed changes
@anikethsaha
Copy link
Copy Markdown
Member

anikethsaha commented Apr 10, 2020

I agree with @Koooooo-7 , we cant show testing fixture in docs.
@aenniw can you make those files just for the testing. try added script here.
that file is copy-pasting content from the original docs. so after the copy-pasting, you can add function to create a file and write the content as you need or in some other way whichever you prefer.
I know its a bit of extra work but we don't have strong test. and the issue addressing in this PR is quite common we noticed in chats. so it would be great to have a good setup and it will help in reviewing this better

@aenniw
Copy link
Copy Markdown
Contributor Author

aenniw commented May 11, 2020

Extracted test specific blocks from docs, updated cypress setup.js to inline content before predefined tag

@aenniw aenniw requested a review from anikethsaha May 11, 2020 20:26
@jthegedus
Copy link
Copy Markdown
Contributor

jthegedus commented May 12, 2020

Looking good

anikethsaha
anikethsaha requested changes May 12, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@anikethsaha anikethsaha 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 to me.
can you fix the CI and we are good to go.

@anikethsaha anikethsaha requested a review from Koooooo-7 May 12, 2020 05:24
@aenniw aenniw force-pushed the bug/791 branch 2 times, most recently from d94db2b to 5770bad Compare May 12, 2020 16:58
@aenniw
Fix out of order embedding of files
661a702 
- added double quote as attribute marker
- fixed inject order of embedded files
- added e2e test covering order of embedded files
@aenniw
Copy link
Copy Markdown
Contributor Author

aenniw commented May 12, 2020

CI fixed by updating reference img as new lines were added to docs via PR and it wasn't updated...

@trusktr
Copy link
Copy Markdown
Member

trusktr commented Jan 12, 2022

Unfortunately (or fortunately, depending on your view) the test system changed from cypress to playwright, making a lot of conflicts in the PR. We need to update this to playwright.

trusktr
trusktr requested changes Jan 12, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@trusktr trusktr left a comment

Choose a reason for hiding this comment

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

needs conflicts resolved, moving from cypress to playwright.

@sy-records sy-records mentioned this pull request Oct 30, 2022
Merged
@sy-records sy-records closed this Jul 24, 2025
@sy-records
Copy link
Copy Markdown
Member

sy-records commented Jul 24, 2025

There are too many conflicts, so it is temporarily closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@trusktr trusktr trusktr requested changes

@anikethsaha anikethsaha anikethsaha approved these changes

@Koooooo-7 Koooooo-7 Awaiting requested review from Koooooo-7

Assignees

@anikethsaha anikethsaha

Labels

pinned This is to pinned the PR/Issue in order to keep it open

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@aenniw @anikethsaha @jthegedus @trusktr @sy-records @Koooooo-7 @jhildenbiddle