-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
Added capability to add css class and id to images + links + refactoring #820
Conversation
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. |
@arelstone can you please resolve the conflicts and try to simplify the diff if possible. I will review this soon. Its a good enhancement. |
@anikethsaha I've resolved the conflicts that is suppose to come after 10 months of idling on any pr. I'm not really sure how to simplify the diff any more. I know it seems like a lot of changes, but it really isn't. I basically just copy-pasted each of the methods into a new file. I've been thinking about doing some kind of attribute parser, but this parser will need to have a lot of special cases for attrs like: export const attributeParser = config => {
return Object.entries(config)
.map(([attr, value]) => {
if (attr === 'size') {
return convertSizeAttr(value)
}
return `${attr}="${value}"`
})
}
const convertSizeAttr = size => {
const [width, height] = size.split('x')
if (height && width) {
return `width="${width}" height="${height}"`
}
return `width="${width}" height="${width}"`
} ...of-course this needs to have added a lot more rules for the ones listed above. Again I'm not sure that this is the way to do it. It would just be to move some business logic into a new file.. |
There was a problem hiding this 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 , left few suggestions
@arelstone this PR is so good 🎉 . And many thanks for adding unit tests. 💯 About the CI error, it needs some changes with new test cases in e2e. if you want to update them, you can go ahead (refer other test cases) else I will do it after merging this.
also, if would be nice if you can do this type of refactoring for other parts of the code as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As @anikethsaha said, this is a good PR. Cheers for the effort.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work.... 💯
looking forward to more contributions from you🎉
```md | ||
![logo](https://docsify.js.org/_media/icon.svg ':id=someCssId') | ||
``` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this works for anchors too, then it feels as if this documentation should be in a section that covers both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I supporse it works for anchors too. Havn't testes it. This pr wasnt with anchors in mind. If it works, Feel free to update the docs
Summary
guide -> helpers
to have an image sectionWhat kind of change does this PR introduce? (check at least one)
If changing the UI of default theme, please provide the before/after screenshot:
Does this PR introduce a breaking change? (check one)
If yes, please describe the impact and migration path for existing applications:
The PR fulfills these requirements:
fix #xxx[,#xxx]
, where "xxx" is the issue number)You have tested in the following browsers: (Providing a detailed version will be better.)
If adding a new feature, the PR's description includes:
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:
lib
directory.