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 horizontal spacing between the icon and text in the custom icon list #2352

Closed
SPWwj opened this issue Aug 5, 2023 · 15 comments · Fixed by #2375
Closed

Add horizontal spacing between the icon and text in the custom icon list #2352

SPWwj opened this issue Aug 5, 2023 · 15 comments · Fixed by #2375
Labels
c.Feature 🚀 d.easy pr.Enhancement 📈 Enhancement to an existing feature

Comments

@SPWwj
Copy link
Contributor

SPWwj commented Aug 5, 2023

Please confirm that you have searched existing issues in the repo

Yes, I have searched the existing issues

Any related issues?

Nil

What is the area that this feature belongs to?

Author Usability, Reader Usability, Syntax

Is your feature request related to a problem? Please describe.

From the PR discussion here:

A feature is needed to let users adjust spacing between customized icons and content, accommodating diverse user preferences.

Describe the solution you'd like

Some ideas:

Introduce an i-spacing attribute to customize the spacing between an icon and its associated list content.

Example Usage:

* Item 1 {icon="/images/deer.jpg" i-width="60px" i-spacing="0.35em"}
* Item 2 { i-spacing="0.55em"}
* Item 3 { i-spacing="0.75em"}

Sample output:
image

Describe alternatives you've considered

No response

Additional context

No response

@SPWwj SPWwj added c.Feature 🚀 pr.Enhancement 📈 Enhancement to an existing feature labels Aug 5, 2023
@Ashleeey1224
Copy link
Contributor

hi, currently I'm learning how to do first PR for a open resource repo, may I take this issue to work on?
Thank you:)

@Ashleeey1224
Copy link
Contributor

I'm going to work on this issue, thxx

@SPWwj
Copy link
Contributor Author

SPWwj commented Oct 18, 2023

@Ashleeey1224
Copy link
Contributor

Ashleeey1224 commented Oct 18, 2023 via email

@Ashleeey1224
Copy link
Contributor

Ashleeey1224 commented Oct 19, 2023

Several Questions wanna ask:

  1. Is the icon considered as "emoji" and "image"? or only image?
  2. Where I should put image file: If I want to test image, where I should put? Based on the sample output and previous PR, the relative path is "/images/deer.jpg". And I also found images file under docs
  3. When I try to load image from "/docs/images", console shows:
 warn: You might have an invalid intra-link! Ignore this warning if it was intended.
'/contents/docs/images/deer.jpg' found in file '/Users/Sammy/Desktop/markbind/contents/topic1.md'

Is it appropriate?

Thaxx for reply :D

@Ashleeey1224
Copy link
Contributor

Hi, I think I figure it out, may I open a PR?

Ashleeey1224 added a commit to Ashleeey1224/markbind that referenced this issue Oct 25, 2023
* Add a new list item attribute called: i-spacing; 
* It allows user to manually change the spacing between icon and text; 
* Issue MarkBind#2352
Ashleeey1224 added a commit to Ashleeey1224/markbind that referenced this issue Oct 25, 2023
@SPWwj
Copy link
Contributor Author

SPWwj commented Oct 27, 2023

Hi, I think I figure it out, may I open a PR?
@tlylt any comments?

@Ashleeey1224
Copy link
Contributor

If it is okay, may I open a draft pr and get some feedback first?

@tlylt
Copy link
Contributor

tlylt commented Oct 28, 2023

If it is okay, may I open a draft pr and get some feedback first?

Sure, @Ashleeey1224. I haven’t got the chance to understand the context here, but please feel free to raise a PR, will take a look afterwards.

Ashleeey1224 added a commit to Ashleeey1224/markbind that referenced this issue 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 issue 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 issue Oct 28, 2023
Ashleeey1224 added a commit to Ashleeey1224/markbind that referenced this issue 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.
@tlylt
Copy link
Contributor

tlylt commented Nov 13, 2023

@SPWwj, looking at your referenced PR discussion, it seems like it wasn't simply adding a feature to let users configure the horizontal space, but rather:

  • is the default spacing currently provided OK for most cases?
  • if not, should we either
    • investigate a way to "automatically scale horizontal spacing between icon and content" quoting @ang-zeyu verbatim
    • or
    • add a simple way for users to adjust the spacing directly (should be what you prefer (?) I suppose)

Could you help me verify if you recall the discussion?

@tlylt
Copy link
Contributor

tlylt commented Nov 13, 2023

@damithc quick question: Per #2316 (comment)
Have you needed to adjust the spacing, or was it OK for your usage so far?

@SPWwj
Copy link
Contributor Author

SPWwj commented Nov 13, 2023

Could you help me verify if you recall the discussion?

Hi @tlylt, from @ang-zeyu's feedback, the icon spacing seems a bit too tight for his preference. He suggests implementing an i-spacing configuration for greater flexibility or spacing automatically according to width of the icon, up to a certain maximum for less flexibility.

Would it be better if we have some sample screenshots showing the different icon spacing on different screens? This would help us determine the optimal display. What are your thoughts, @Ashleeey1224 and @tlylt?

@tlylt
Copy link
Contributor

tlylt commented Nov 13, 2023

Would it be better if we have some sample screenshots showing the different icon spacing on different screens? This would help us determine the optimal display. What are your thoughts, @Ashleeey1224 and @tlylt?

Yes would be nice to see them to compare the approaches. @Ashleeey1224 would you be able to help?

@damithc
Copy link
Contributor

damithc commented Nov 13, 2023

@damithc quick question: Per #2316 (comment)
Have you needed to adjust the spacing, or was it OK for your usage so far?

@tlylt Not specifically, but I'm fine to provide this flexibility to authors. It may be useful in some situations.

@kaixin-hc
Copy link
Contributor

To clarify the concern behind why the PR was closed, it was regarding whether the spacing should be automatic (based on width of the icon) or able to be manually modified (as in the PR).

I feel adding the manual modification is okay because it still has a default value, so the work to the average user is minimal - while implementing automatic spacing could lead to some sort of edge case. We could also do changes to this incrementally - merge manual spacing first, and if it makes sense to implement additional automatic spacing, that can be done in another PR afterwards.

yucheng11122017 added a commit to Ashleeey1224/markbind that referenced this issue Mar 8, 2024
yucheng11122017 added a commit to Ashleeey1224/markbind that referenced this issue Mar 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c.Feature 🚀 d.easy pr.Enhancement 📈 Enhancement to an existing feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants