-
Notifications
You must be signed in to change notification settings - Fork 33
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
Awesome list preperation #12
Comments
Awesome-lint runFixed various issues. Many issues still remain but are invalid. Examples:
|
So, the only thing missing is the 2nd PR review, is that correct? |
Yes, I think so 🙂 |
The guidelines have the following statement regarding the table of contents: "Should only have one level of nested lists, preferably none." Maybe we should remove the link to the top heading so we can shift up everything in the list. |
Actually, that limitation would also cause problem with the "Data Formats" as well. If we follow the guidelines, JSON, CSV and XML would need to |
Also, if you need some inspirations for a review, it seems like the recent PR named "Add Google Cloud" failed to apply the following rule (last 2 sentences):
Basically, the contributing link should be in a seperate section named "Contributing" (not a subsection) and the section "About This Document" shouldn't be part of the Table of Contents and renamed to "Footnotes" based on the next section of the guildelines. |
Also iirc I think I held off applying because of the symbology. I think the symbology is theoretically not allowed... |
It would be allowed at the end in the Footnotes section. Considering that the icons have a tooltip when hovering on them, I personally wouldn't mind putting the symbology at the end if that means that the list becomes eligible. There's always the option to ask for an exception, but I doubt that we'd get it. |
Not sure why this was closed down... I need to do this |
@DecimalTurn you are correct, this is a valid point. |
I discovered recently that if you write "Fixes" or "Closes" in front an issue inside the description of a PR, GitHub will link them together and close the issue when the PR is merged. EDIT: This also applies to commit message of commits inside the branch for the PR. PS: I re-read my last message and realized my last message was missing a negative in "wouldn't mind", but the setup of the sentence made it clear what I meant luckily. |
Correct, I did know about this, total oversight from my part because "fixes" in that sentence implies something else to me xD I altered the awesome list with the aforementioned changes of moving symbology to footnotes last night, and prepared a fork @ https://github.com/sancarn/awesome |
Nice. Only need to adjust the TOC and I think we are good to go! |
Closeout in progress sindresorhus/awesome#3153 |
FYI, regarding the Symbology section, I just noticed that this awesome list has a legend section right after the TOC, so we could ask if we can do the same. |
Glancing at the Linter validation rules I think moving the symbols to the end of the line will fix the linting issues, it looks like it wants the structure |
Indeed. I don't think that is the right course of action though... I think the linter needs a PR to allow it to consider this option. What are your thoughts? |
Symbols at the start seems preferable for me as well. For the linter, I'm guessing we'd have to do the PR ourselves otherwise it won't happen. |
Probably, yes 😅. Looks like it would be modifying this file: https://github.com/sindresorhus/awesome-lint/blob/d83d0361be944593b27d3060942a6a7dce33216d/rules/list-item.js#L141 |
Actually I think the problem is more here: So currently it continuously discards till it hits a link. Better would be looping through until we hit a |
Btw, If we have to drop the wrapper around images ( Note to selfReplace: |
By doing that and some other changes, I managed to make a few changes that made the linter checks pass. I'll make a PR later this weekend unless you think I shouldn't. |
I can live with it but it's less awesome 😅 Not really sure if we should make concessions for something which decreases quality of the list... |
Links to sindresorhus#199 Also see sancarn/awesome-vba#12 for reference
PR Requirements
PR Title: "Add VBA"
PR addition: "VBA/VB6 - Visual Basic for Applications (VBA) is an implementation of Microsoft's event-driven programming language Visual Basic 6.0 (VB6) built into most desktop Microsoft Office applications"
unicorn
to PR descriptionAuthor requirements
Requirements
The text was updated successfully, but these errors were encountered: