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

Drop glyphicons and replace it by already existing icons in icons.css #1420

Merged

Conversation

hexaltation
Copy link
Collaborator

@hexaltation hexaltation commented Feb 4, 2025

Context

Bump bootstrap from 3.4.1 to 5.3.3 lead to a bug.
In v 4.0.0 bootstrap dropped glyphicon usage.
Grist-core code have some occurences of glyphicon.

$ grep -rin glyphicon
client/lib/koForm.js:337:    this.optionButton("left", dom('span.glyphicon.glyphicon-align-left'),
client/lib/koForm.js:339:    this.optionButton("center", dom('span.glyphicon.glyphicon-align-center'),
client/lib/koForm.js:341:    this.optionButton("right", dom('span.glyphicon.glyphicon-align-right'),
client/lib/koForm.js:482:            dom('span.kf_drag_indicator.glyphicon.glyphicon-option-vertical') :
client/lib/koForm.js:487:            return dom('span.drag_delete.glyphicon.glyphicon-remove',
client/components/DocConfigTab.js:19:      dom('span.glyphicon.glyphicon-check'),
client/components/ValidationPanel.js:66:            dom('div.validation_trash.glyphicon.glyphicon-remove',
client/components/ValidationPanel.js:71:            1, dom('div.glyphicon.glyphicon-tag.config_icon'),
client/components/ColumnFilters.css:69:.g-glyphicon-tristate {
client/components/RecordLayoutEditor.js:128:    dom('div.g_record_delete_field.glyphicon.glyphicon-eye-close',
client/widgets/AttachmentsWidget.ts:193:const cssAttachmentIcon = styled('div.glyphicon.glyphicon-paperclip', `

Proposed solution

Remove Glyphicon calls and replace it by corresponding icons in static/icon/icons.css

Related issues

Fixes #1412

Has this been tested?

  • 👍 yes, I added tests to the test suite
  • 💭 no, because this PR is a draft and still needs work
  • 🙅 no, because this is not relevant here
  • 🙋 no, because I need help

Glyphicons where removed from bootstrap in 4.0.0.
Bootstrap bump from 3.x.x to 5.x.x leads at least to missing icons.
Use the one in `static/icon/icons.css`
@hexaltation hexaltation marked this pull request as ready for review February 5, 2025 14:51
app/client/lib/koForm.js Outdated Show resolved Hide resolved
app/client/lib/koForm.js Outdated Show resolved Hide resolved
@georgegevoian georgegevoian self-requested a review February 5, 2025 15:57
app/client/components/DocConfigTab.js Outdated Show resolved Hide resolved
app/client/lib/koForm.js Outdated Show resolved Hide resolved
app/client/components/RecordLayoutEditor.js Show resolved Hide resolved
@paulfitz
Copy link
Member

paulfitz commented Feb 5, 2025

and also remove from it glyphicons and replace by grist icons
app/client/lib/koForm.js Outdated Show resolved Hide resolved
app/client/lib/koForm.js Outdated Show resolved Hide resolved
@georgegevoian
Copy link
Contributor

For the most recent browser test failure, you can just remove the failing test (related to the now-removed validation feature). If you grep the codebase for validationsTool, there's a few other small things you can remove as well.

@georgegevoian georgegevoian added the preview Launch preview deployment of this PR label Feb 6, 2025
Copy link
Contributor

github-actions bot commented Feb 6, 2025

Deployed commit 44a2f31d031b9c3d36ca938e6545005b398c0b01 as https://grist-hexaltation-grist-core-drop-glyphicons-for-octicons.fly.dev (until 2025-03-08T15:28:27.605Z)

@paulfitz
Copy link
Member

paulfitz commented Feb 6, 2025

Haven't looked at code, but not seeing paperclip in the preview?

Screenshot from 2025-02-06 10-35-06

Copy link
Contributor

github-actions bot commented Feb 6, 2025

Deployed commit 12ba314cde18b8716f1babf165179907865a993d as https://grist-hexaltation-grist-core-drop-glyphicons-for-octicons.fly.dev (until 2025-03-08T16:09:55.056Z)

Copy link
Contributor

github-actions bot commented Feb 6, 2025

Deployed commit 5d046283e3a23731cfcc37f10e11141d867c44ab as https://grist-hexaltation-grist-core-drop-glyphicons-for-octicons.fly.dev (until 2025-03-08T16:20:18.406Z)

@hexaltation
Copy link
Collaborator Author

For the most recent browser test failure, you can just remove the failing test (related to the now-removed validation feature). If you grep the codebase for validationsTool, there's a few other small things you can remove as well.

Thanks
it's now cleaned

Copy link
Contributor

github-actions bot commented Feb 6, 2025

Deployed commit 61fb981659c229c8fa5054c69d4c12a57afa1243 as https://grist-hexaltation-grist-core-drop-glyphicons-for-octicons.fly.dev (until 2025-03-08T16:33:21.584Z)

Copy link
Contributor

github-actions bot commented Feb 6, 2025

Deployed commit dd05dcd1fd5dd1785e3ffee4a87ff84ac513800c as https://grist-hexaltation-grist-core-drop-glyphicons-for-octicons.fly.dev (until 2025-03-08T16:54:36.538Z)

@georgegevoian georgegevoian self-requested a review February 6, 2025 17:00
Copy link
Contributor

@georgegevoian georgegevoian left a comment

Choose a reason for hiding this comment

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

Thanks @hexaltation.

@georgegevoian georgegevoian merged commit 2708a3a into gristlabs:main Feb 6, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-UX/UI gouv.fr preview Launch preview deployment of this PR
Projects
Status: Done
Status: To Review
Development

Successfully merging this pull request may close these issues.

Attachment icon is missing
3 participants