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

Customizing list icons: give a way to apply to current item only #2454

Merged
merged 88 commits into from
Apr 7, 2024

Conversation

KevinEyo1
Copy link
Contributor

@KevinEyo1 KevinEyo1 commented Mar 11, 2024

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:
Fixes #2405
Modified CustomListIconProcessor.ts to account for new once variable.
Updated lists UG file to include new feature.
Update testLists to test new feature.

Anything you'd like to highlight/discuss:
Implemented such that it will not overwrite previous icon saved.

1. Customization + one-off + none
* Item A { icon="fas-file-code" }
* Item B { icon="glyphicon-education" once=true }
  * Sub-item B1
  * Sub-item B2
  * Sub-item B3 
    * Sub-sub-item B3.1 { icon="./images/deer.jpg" i-width="50px" }
    * Sub-sub-item B3.2
  * Sub-item B4 
* Item C

image

Currently once icon being in the first item, AND the next item having no icon, results in a visual issue.
To note that the final item with a new icon is working as intended.

1. One-off + none + customization
* Item A { icon="glyphicon-education" once=true }
* Item B 
  * Sub-item B1 { icon="fas-file-code" }
  * Sub-item B2
  * Sub-item B3 
    * Sub-sub-item B3.1 { icon="./images/deer.jpg" i-width="50px" }
    * Sub-sub-item B3.2
  * Sub-item B4 
* Item C { icon="fas-file-code" }

image

Testing instructions:
Serve the folder at markbind\packages\cli\test\functional\test_site and navigate to testList.html

Proposed commit message: (wrap lines at 72 characters)
List icons: icons for specific item only

Unordered list icons are used in subsequent items to avoid
duplication.

MarkBind lacks a way to indicate icons for a specific item
only, limiting the flexibility of icons.

Creating a variable to indicate once icons allow for user
to have greater flexibility while preserving the current nature
of list icons.

Let's add a once variable that users can set when adding
icons to items in the list, such that future items won't inherit
the icon, and it will also not overwrite previous icons saved.

This approach directly allows users to pick specific icons for
specific items, while keeping the inheritance nature of other icons
intact.


Checklist: ☑️

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

Reviewer checklist:

Indicate the SEMVER impact of the PR:

  • Major (when you make incompatible API changes)
  • Minor (when you add functionality in a backward compatible manner)
  • Patch (when you make backward compatible bug fixes)

At the end of the review, please label the PR with the appropriate label: r.Major, r.Minor, r.Patch.

Breaking change release note preparation (if applicable):

  • To be included in the release note for any feature that is made obsolete/breaking

Give a brief explanation note about:

  • what was the old feature that was made obsolete
  • any replacement feature (if any), and
  • how the author should modify his website to migrate from the old feature to the replacement feature (if possible).

@yucheng11122017 yucheng11122017 force-pushed the master branch 2 times, most recently from cb84513 to 69ec838 Compare April 5, 2024 06:21
@KevinEyo1 KevinEyo1 marked this pull request as draft April 6, 2024 04:26
@KevinEyo1 KevinEyo1 marked this pull request as ready for review April 6, 2024 16:41
Copy link
Contributor

@yucheng11122017 yucheng11122017 left a comment

Choose a reason for hiding this comment

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

Small nits

// If an item has a specified icon, that icon and its attributes will be saved and used
// for it and for subsequent items at that level to prevent duplication of icons
// attribute declarations.
// If an item is once, its icons and/or attributes will only be used for that item.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// If an item is once, its icons and/or attributes will only be used for that item.
// If `once` is true, its icons and/or attributes will only be used for that item.

if (!iconAttrValue.addIcons) {
return;
}
// for items after first item, if first item is once, no previous icon
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// for items after first item, if first item is once, no previous icon
// for subsequent items, if first item is once, there is no previous icon

if (iconAttrValue.iconAttrs?.icon === undefined && iconAttrValue.iconAttrs?.text === undefined) {
// There is no previous icon and no previous text
const nodeIconAttrs = getIconAttributes(node, renderMdInline);
// Save if current has icon or text, and it is not once
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Save if current has icon or text, and it is not once
// Save if current item has icon or text, and it is not once

}
}

// update only if current has icon/text or previous has saved icon/text
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// update only if current has icon/text or previous has saved icon/text
// update only if current item has icon/text or previous item has saved icon/text

return;
}
// for items after first item, if first item is once, no previous icon
// so future items that are not once will need to be saved
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// so future items that are not once will need to be saved
// so future attributes that are not once will need to be saved

Copy link
Contributor

@yucheng11122017 yucheng11122017 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for all the work @KevinEyo1

@yucheng11122017 yucheng11122017 merged commit fa48343 into MarkBind:master Apr 7, 2024
9 of 11 checks passed
@github-actions github-actions bot added the r.Minor Version resolver: increment by 0.1.0 label Apr 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
r.Minor Version resolver: increment by 0.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Customizing list icons: give a way to apply to current item only
6 participants