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

Favicons #4301

Closed
wants to merge 2 commits into from
Closed

Favicons #4301

wants to merge 2 commits into from

Conversation

jonnymccullagh
Copy link

Proposed change

The bookmarks component uses a lot of horizontal space and I was looking for a mechanism to add a row of icons each linking to a corresponding URL. I have copied much of the existing bookmarks code to create a favicons component controlled by additions to a favicons.yaml config file. The example screenshot below shows some favicons under the traditional bookmarks:
favicons

Discussion here

Please feel free to suggest changes.

Type of change

  • New service widget
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Documentation only
  • Other (please explain)

Checklist:

  • If applicable, I have added corresponding documentation changes.
  • If applicable, I have reviewed the feature and / or service widget guidelines.
  • I have checked that all code style checks pass using pre-commit hooks and linting checks.
  • If applicable, I have tested my code for new features & regressions on both mobile & desktop devices, using the latest version of major browsers.

@shamoon
Copy link
Collaborator

shamoon commented Nov 15, 2024

Thanks but this does not meet the threshold for new feature requests as outlined above. This can be readdressed in the future if that changes.

@shamoon shamoon closed this Nov 15, 2024
@markalex2209
Copy link

@shamoon, if I understand correctly, threshold for FR is met now.

@jonnymccullagh
Copy link
Author

@shamoon, if I understand correctly, threshold for FR is met now.

@markalex2209 is the threshold met because of votes on the discussion here ? I am new to this project.

@markalex2209
Copy link

I'm also new to this project, but service widget guidelines mention:

Please only submit widgets that target a feature request discussion with at least 10 'up-votes'. The purpose of this requirement is to avoid the addition (and maintenance) of service widgets that might only benefit a small number of users.

And I noticed, that your discussion already has +10, so brought this up.

@shamoon
Copy link
Collaborator

shamoon commented Dec 5, 2024

  1. We removed the exact number requirement for non-widget FRs. So while Im not really sure that 10 votes is indicative of a widespread interest for this, we take these on a case-by-case basis.
  2. More importantly, the issue with this PR (now that I look at it) is that I am not happy with this approach to the implementation. This PR is nearly 100% duplicated code from elsewhere and requires an entire new config. There are other issues too (debug lines in the PR, poor aesthetics, 'favicon' is not accurate, etc). So, I'm sorry if this comes across as obnoxious but rather than request this PR be re-done I went ahead and implemented it in Enhancement: icons-only bookmarks style #4384

@jonnymccullagh
Copy link
Author

thank you @markalex2209 for your input and thank you @shamoon for implementing the change. The fork, PR were for my own use but now looking forward to seeing this implemented for everyone. Cheers.

Copy link
Contributor

github-actions bot commented Jan 5, 2025

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new discussion for related concerns. See our contributing guidelines for more details.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 5, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants