Skip to content

Fix #791 - #866

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

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

Conversation

@aenniw

Copy link
Copy Markdown
Contributor

Signed-off-by: Martin Mihálek aenniw@gmail.com

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
ContributorAuthor

Addresses #791

@stale

staleBot commented Feb 4, 2020

Copy link
Copy Markdown

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.

@stalestaleBot added the wontfix label Feb 4, 2020
@stalestaleBot removed the wontfix label Feb 4, 2020
@anikethsahaanikethsaha 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

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

@anikethsahaanikethsaha self-assigned this Feb 5, 2020
@anikethsaha
anikethsaha requested a review from a teamFebruary 5, 2020 18:37
alexis- added a commit to supermemo-wiki/docsify that referenced this pull request Feb 8, 2020
alexis- added a commit to supermemo-wiki/SuperMemoAssistant.Wiki that referenced this pull request Feb 8, 2020
@aenniwaenniw 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
@aenniwaenniw reopened this Feb 23, 2020
@anikethsaha

Copy link
Copy Markdown
Member

can you resolve the conflicts

@aenniw

Copy link
Copy Markdown
ContributorAuthor

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

@trusktrtrusktr left a comment

Copy link
Copy Markdown
Member

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
ContributorAuthor

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
ContributorAuthor

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

@anikethsahaanikethsaha left a comment

Copy link
Copy Markdown
Member

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

Comment threadcypress/support/commands.js
Comment threaddocs/_media/delayed.md
Comment threaddocs/_media/non-delayed.md
@anikethsaha
anikethsaha requested review from a team and removed request for a teamApril 10, 2020 09:26

@Koooooo-7Koooooo-7 left a comment

Copy link
Copy Markdown
Member

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.

Comment threaddocs/embed-files.md
@anikethsaha

Copy link
Copy Markdown
Member

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
ContributorAuthor

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

@aenniw
aenniw requested a review from anikethsahaMay 11, 2020 20:26
@jthegedus

Copy link
Copy Markdown
Contributor

Looking good

@anikethsahaanikethsaha left a comment

Copy link
Copy Markdown
Member

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.

@aenniw
aenniwforce-pushed the bug/791 branch 2 times, most recently from d94db2b to 5770badCompareMay 12, 2020 16:58
- 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
ContributorAuthor

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

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.

@trusktrtrusktr left a comment

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member

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

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

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

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