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

Add Icon and Media Support to List - Resolves #906 and #2272 #2316

Merged
merged 101 commits into from Aug 5, 2023

Conversation

SPWwj
Copy link
Contributor

@SPWwj SPWwj commented Jun 28, 2023

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • DevOps
  • Improve developer experience
  • Others, please explain:

Overview of changes:
image
image
image
image
image

This PR resolves issue #906 by adding support for custom icons in lists, and also addresses #2272 for additional icon support.

Anything you'd like to highlight/discuss:

  • Kindly note, for emojis, please avoid the :: symbol, due to our markdown decode pipeline's internal design. Whether we'll rectify this in future is a topic for separate consideration.

  • markdown-it-icons.ts has been refactored for better maintainability and can be reused by this Unordered Icon list to improve code consistency. However, all its functionalities remain unchanged.

  • The suggestion of multi-icon settings, {icons=":thumbs_up: :light_bulb:"}, discussed in issue Support custom icons for lists #2272, is worth considering. Nonetheless, I believe excessive abstraction in markdown might not be optimal. We can delve deeper into this subject in PR Introducing Custom Markdown Syntax Pipeline and QR Code Component #2315. This feature could be potentially addressed in a minor patch.

  • To create a tightly nested list, you can use the following method:

    * Item 1 <br>
      Item 1 line 2{icon="/images/deer.jpg" i-width="60px" height="17px" class="rounded"}
    * Item 2
      Item 2 Continue
      * Item 2.1 {icon="fas-question-circle" i-class="badge rounded-pill mb-1 bg-success text-white"}
      * Item 2.2 {i-class="badge rounded-pill mb-1 bg-danger text-white"}
      * Item 2.3 
        * Item 2.3.1 {icon="fas-question-circle" i-class="badge rounded mb-1 bg-danger text-white"}
    * Item 3
      * Item 3.1 
      * Item 3.2 {i-class="badge rounded-pill mb-1 bg-primary text-white"}
      * Item 3.3 {icon="fas-compass" i-class="badge rounded-pill mb-1 bg-warning text-white}
      * Item 3.4
    * Item 4
      * Item 4.1
        * Item 4.1.1
      * Item 4.2 
    
    
    image

Testing instructions:

  1. Confirm that you have the correct branch for this pull request checked out on your local environment.
  2. Establish a new MarkBind project using the markbind init command.
  3. Experiment with the list using the provided syntax from above.

Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No unrelated changes

@yucheng11122017
Copy link
Contributor

Hi @SPWwj it looks p cute!
Jic you forgot, don't forget to document this feature and add tests! Will make it a lot easier to review

@SPWwj
Copy link
Contributor Author

SPWwj commented Jun 29, 2023

Hi @SPWwj it looks p cute!

Jic you forgot, don't forget to document this feature and add tests! Will make it a lot easier to review

Roger🫡will do it tmr😁

@damithc
Copy link
Contributor

damithc commented Jun 29, 2023

Jic you forgot, don't forget to document this feature and add tests! Will make it a lot easier to review

@SPWwj Yup, in fact I would like to see the documentation first, so that I can evaluate the PR from the user POV. Perhaps the tests can be put on hold until we have firmed up the feature behavior.

@SPWwj
Copy link
Contributor Author

SPWwj commented Jun 30, 2023

Jic you forgot, don't forget to document this feature and add tests! Will make it a lot easier to review

@SPWwj Yup, in fact I would like to see the documentation first, so that I can evaluate the PR from the user POV. Perhaps the tests can be put on hold until we have firmed up the feature behavior.

The documentation has been updated, potentially revealing two new bugs.😂

Copy link
Contributor

@damithc damithc left a comment

Choose a reason for hiding this comment

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

Just some quick comments, just for the UG only.

docs/userGuide/syntax/lists.md Outdated Show resolved Hide resolved
docs/userGuide/syntax/lists.md Outdated Show resolved Hide resolved
docs/userGuide/syntax/lists.md Outdated Show resolved Hide resolved
docs/userGuide/syntax/lists.md Outdated Show resolved Hide resolved
docs/userGuide/syntax/lists.md Outdated Show resolved Hide resolved
docs/userGuide/syntax/lists.md Outdated Show resolved Hide resolved
docs/userGuide/syntax/lists.md Outdated Show resolved Hide resolved
docs/userGuide/syntax/lists.md Outdated Show resolved Hide resolved
docs/userGuide/syntax/lists.md Outdated Show resolved Hide resolved
docs/userGuide/syntax/lists.md Outdated Show resolved Hide resolved
@yucheng11122017
Copy link
Contributor

Hi @SPWwj, thanks for the changes!
Please do correct all the linting errors though makes it a lot easier to see what is changed :) Thanks!

@damithc
Copy link
Contributor

damithc commented Jul 2, 2023

@SPWwj While implementing this feature, keep in mind the possibility of a new feature for customizing ordered lists: number style (e.g., A,B,C i, ii, iii etc.) and presentation (font weight, color etc.). It is ideal if that feature can be similar to this (both from user POV and dev POV).

@SPWwj
Copy link
Contributor Author

SPWwj commented Jul 2, 2023

Hi @SPWwj, thanks for the changes! Please do correct all the linting errors though makes it a lot easier to see what is changed :) Thanks!

Fixed 👍

@SPWwj
Copy link
Contributor Author

SPWwj commented Jul 2, 2023

@SPWwj While implementing this feature, keep in mind the possibility of a new feature for customizing ordered lists: number style (e.g., A,B,C i, ii, iii etc.) and presentation (font weight, color etc.). It is ideal if that feature can be similar to this (both from user POV and dev POV).

I realized that I can transform a list into any form as needed.😂

docs/userGuide/syntax/lists.md Outdated Show resolved Hide resolved
docs/userGuide/syntax/lists.md Outdated Show resolved Hide resolved
docs/userGuide/syntax/lists.md Outdated Show resolved Hide resolved
@SPWwj SPWwj requested a review from ang-zeyu August 1, 2023 06:52
Copy link
Contributor

@ang-zeyu ang-zeyu left a comment

Choose a reason for hiding this comment

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

Just documentation nits, don't have anything else to add on the implementation:

docs/userGuide/syntax/lists.md Outdated Show resolved Hide resolved
docs/userGuide/syntax/lists.md Outdated Show resolved Hide resolved
docs/userGuide/syntax/lists.md Outdated Show resolved Hide resolved
docs/userGuide/syntax/lists.md Outdated Show resolved Hide resolved
docs/userGuide/syntax/lists.md Outdated Show resolved Hide resolved
docs/userGuide/syntax/lists.md Outdated Show resolved Hide resolved

* ##### Heading 3.1: Final Thoughts

Content 3.1: This is a place for final reflections, recommendations, or a look towards future developments or trends.
Copy link
Contributor

Choose a reason for hiding this comment

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

similarly, this example could be more concise for the reader. (e.g. heading 1.1, 3, 3.1 seems unnecessary)

Copy link
Contributor

Choose a reason for hiding this comment

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

this was added back in your revert commit

docs/userGuide/syntax/lists.md Outdated Show resolved Hide resolved
@SPWwj SPWwj requested a review from ang-zeyu August 4, 2023 21:07
@ang-zeyu ang-zeyu merged commit d6ad99d into MarkBind:master Aug 5, 2023
6 checks passed
@ang-zeyu
Copy link
Contributor

ang-zeyu commented Aug 5, 2023

I've added a commit message, but try to practice writing them. There is a link in the PR template to a guide. Concise messages go a long way in documenting changes.

Github has issue closing keywords you can use to automatically close issues too, link in the template.

In case I forget, self reminder to raise a follow up for automatically scale horizontal spacing between icon and content.

@damithc
Copy link
Contributor

damithc commented Sep 1, 2023

This feature in action https://nus-cs2103-ay2324s1.github.io/website/admin/appendixD-help.html

Thanks again to @SPWwj for authoring, and @ang-zeyu for guiding, this massive and long-running PR.

Ashleeey1224 added a commit to Ashleeey1224/markbind that referenced this pull request Oct 28, 2023
- Allow users to modify icon's size, width, height, and class for customizing icon attributes while also providing the option to adjust spacing between the icon and text for enhanced user experience.

- Added "i-spacing" attribute to ICON_ATTRIBUTES and interface IconAttributes in CustomListIconProcessor.ts.

- Implemented logic to check whether the user has defined the i-spacing attribute or not.

- Updated createIconSpan function to incorporate i-spacing attribute.

- Updated getIconAttributes function to retrieve i-spacing attribute.

- This consolidation of attributes maintains code consistency and cleanliness. And it ensures that NodeProcessor.ts can easily access and utilize these attributes without additional complexity.

- Refer issue: MarkBind#2352, MarkBind#2316.
Ashleeey1224 added a commit to Ashleeey1224/markbind that referenced this pull request Oct 28, 2023
- Allow users to modify icon's size, width, height, and class for customizing icon attributes while also providing the option to adjust spacing between the icon and text for enhanced user experience.

- Add "i-spacing" attribute to ICON_ATTRIBUTES and interface IconAttributes in CustomListIconProcessor.ts.

- Implement logic to check whether the user has defined the i-spacing attribute or not.

- Update createIconSpan function to incorporate i-spacing attribute.

- Update getIconAttributes function to retrieve i-spacing attribute.

- It maintains code consistency and cleanliness.

- Refer issue: MarkBind#2352, MarkBind#2316.
Ashleeey1224 added a commit to Ashleeey1224/markbind that referenced this pull request Oct 28, 2023
- Allow users to modify icon's size, width, height, and class for customizing icon attributes while also providing the option to adjust spacing between the icon and text for enhanced user experience.

- Add "i-spacing" attribute to ICON_ATTRIBUTES and interface IconAttributes in CustomListIconProcessor.ts.

- Implement logic to check whether the user has defined the i-spacing attribute or not.

- Update createIconSpan function to incorporate i-spacing attribute.

- Update getIconAttributes function to retrieve i-spacing attribute.

- This consolidation of attributes maintains code consistency and cleanliness.

- Refer issues: MarkBind#2352, MarkBind#2316.
@EltonGohJH EltonGohJH mentioned this pull request Mar 9, 2024
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants