feat(#280): adds icon component with material desing icons #384

latin-panda · 2025-04-25T02:54:47Z

Closes #280

I have verified this PR works in these browsers (latest versions):

What else has been done to verify that this works as intended?

I verified the bundle, and I confirm that the tree-shaking is working:

Including only imported icons
Not increasing bundle size - just minimal.

Test evidence:

Web Form Preview Page

Rank

Multiselect (check icon)

Image upload

Geopoint

Form header

Why is this the best possible solution? Were any other approaches considered?

Easy to add new icons to the project, just import them and add them to the map.
Easy to use. Minimalistic component.
It has baked-in tree-shaking
Wide variety of icons
Well-supported library by opensource community

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

User won't notice significant changes

Do we need any specific form for testing your changes? If so, please attach one.

No

What's changed

Removes icomoon
Adds new dependency with Material Design icons @mdi/js
Adds a new icon component that imports Material Design icons, has variants, and sizes.
- Our lint requires a two-word name for components. That's why the name is IconSVG
Replace all icons with the new component

…ial-icons

changeset-bot · 2025-04-25T02:54:50Z

🦋 Changeset detected

Latest commit: 83df5cd

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@getodk/web-forms	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

latin-panda · 2025-04-25T03:08:23Z

packages/web-forms/src/assets/css/style.scss

@@ -25,8 +25,11 @@
 	--odk-primary-lighter-background-color: var(--p-primary-50);
 	--odk-primary-border-color: var(--p-primary-500);

-	--odk-error-text-color: var(--p-message-error-color);
-	--odk-error-background-color: var(--p-message-error-background);
+	--odk-error-text-color: var(--p-red-600);


I realized that --p-message-error-background is not always available if the message component is unused. Better use the base color variable from PrimeVue

latin-panda · 2025-04-25T03:11:00Z

packages/web-forms/src/assets/css/style.scss

@@ -38,8 +41,6 @@

 	--odk-border-color: var(--p-surface-300);

-	--odk-icon-size: 1.5rem;


This is unlikely to be customized in theming. The primary reason for removing it and letting the IconSVG component handle it is that the SVG's path, which contains the image, is resized using CSS transform.

latin-panda · 2025-04-25T03:15:44Z

packages/web-forms/src/components/FormHeader.vue

+				</Button>
+				<Menu id="overlay_menu" ref="menu" :model="items" :popup="true">
+					<template #item="{ item }">
+						<a class="p-menu-item-link" v-if="item.command != null" @click="(event) => item.command?.({ originalEvent: event, item })">


TypeScript static check doesn't understand that the item.command is already validated in the v-if="item.command != null" 🤦🏽‍♀️ That's why the optional chaining operator

Looks like vuejs/vetur#1212?

Maybe ! instead of ? is slightly more explicit since you know it isn't null at that point but not a big deal.

There's also a lint warning here.

Interesting, the ! didn't fix the TypeScript static check and requires a bit more to make it work. Ultimately, I'm checking item.command is defined before calling it.

I've fixed the GH's lint warning.

latin-panda · 2025-04-25T03:17:03Z

packages/web-forms/src/components/controls/Upload/UploadImageControl.vue

@@ -75,18 +76,7 @@ watchEffect(() => {
 				:disabled="isDisabled"
 				@click="triggerInputField(takePictureInput)"
 			>
-				<svg


Red is the new black 😌

packages/web-forms/src/components/widgets/IconSVG.vue

latin-panda · 2025-04-25T03:22:08Z

packages/web-forms/src/components/widgets/MultiselectDropdown.vue

+			content: '\2713';
+			font-family: system-ui;


This seems close enough to the check mark from Material Icon. It wasn't worth the effort to customize this component to use Material Icons (for now).

It's a safe well supported font, I tested this font in chrome, firefox, safari

latin-panda · 2025-04-25T03:30:19Z

packages/web-forms/src/components/widgets/IconSVG.vue

+} from '@mdi/js';
+
+// Find icons here: https://pictogrammers.com/library/mdi/
+const iconMap: Record<string, string> = {


We don't have that many icons. If it grows a lot eventually, we will extract it to a file; for now, it's good to have it in one place.

latin-panda · 2025-04-25T03:34:05Z

@lognaturel @alyblenkin I need to do another pass of testing, but you can start reviewing.

Please find the screenshots in this section from the PR's description:

lognaturel

Couple of small questions in line!

lognaturel · 2025-04-25T19:11:19Z

packages/web-forms/README.md


-By following the steps above, you should minimize the diff. However, in the JSON file, you may still see changes for properties like `id`, `iconIdx`, `setId`, and `setIdx`.
+- Import the icon from `@mdi/js` in the `Icon` component.


lognaturel · 2025-04-25T19:12:40Z

packages/web-forms/src/components/FormHeader.vue

+				</Button>
+				<Menu id="overlay_menu" ref="menu" :model="items" :popup="true">
+					<template #item="{ item }">
+						<a class="p-menu-item-link" v-if="item.command != null" @click="(event) => item.command?.({ originalEvent: event, item })">


There's also a lint warning here.

packages/web-forms/src/components/widgets/IconSVG.vue

packages/web-forms/README.md

alyblenkin · 2025-04-25T23:20:33Z

Looking good! The comments below are small and not a deal breaker.

On the WF preview page in file upload section, the file icon looks like a different colour.
I also think the file without the lines is a bit less distracting.
I'm not sure why the upload and preview icon look so large in comparison to the rest. I know they are 1.5rem/24px, but they look too big on this page. Maybe the preview eye could use the outline?
I'm tempted to do 22px for all the icons because I know we tried 20 and it was too small.

I also noticed today that there is some inconsistency between the spacing of the icons and the text within the button. I could add another issue for this if that's helpful?

latin-panda · 2025-04-28T19:22:01Z

@alyblenkin @lognaturel I’ve addressed the feedback:

Updated documentation to reference IconSVG instead of Icon.
Resolved lint warning.
Changed the icon color for the document icon and replaced it with a lineless icon (initially searched for "document" but used "file" to find the lineless version).
Resized icons to 20px. Calendar is now bigger, matching 20px.
Centered icons within their containers, which may result in varying distances from the text.

Screenshots

latin-panda added 6 commits April 23, 2025 13:42

Adds material icons library and icon component

4ce9de5

Merge branch 'main' of https://github.com/getodk/web-forms into mater…

a471971

…ial-icons

Replace with new icon component

6769b72

Replace with new icon component image upload and rank

668538c

Removed icomoon

d01c26b

lint

a32be7f

latin-panda added 2 commits April 24, 2025 20:56

changeset

93b710b

Typos

ed24522

latin-panda commented Apr 25, 2025

View reviewed changes

packages/web-forms/src/components/widgets/IconSVG.vue Show resolved Hide resolved

latin-panda commented Apr 25, 2025

View reviewed changes

latin-panda mentioned this pull request Apr 25, 2025

chore: adding icon component to easily use material icons #279

Closed

6 tasks

latin-panda requested review from lognaturel and alyblenkin April 25, 2025 03:31

lognaturel reviewed Apr 25, 2025

View reviewed changes

alyblenkin mentioned this pull request Apr 26, 2025

Change end of form message when edits are enabled getodk/collect#6686

Open

latin-panda added 4 commits April 28, 2025 10:31

Feedback

2d5a9e9

Icon sizes to 20px

14bb968

Icons for group and repeat

6437e99

Fixes lint

83df5cd

latin-panda requested a review from lognaturel April 28, 2025 19:22

lognaturel approved these changes Apr 28, 2025

View reviewed changes

alyblenkin approved these changes Apr 28, 2025

View reviewed changes

latin-panda merged commit ea9674c into main Apr 28, 2025
81 of 85 checks passed

latin-panda deleted the material-icons branch April 28, 2025 19:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(#280): adds icon component with material desing icons #384

feat(#280): adds icon component with material desing icons #384

latin-panda commented Apr 25, 2025 •

edited

Loading

changeset-bot bot commented Apr 25, 2025 •

edited

Loading

latin-panda Apr 25, 2025

latin-panda Apr 25, 2025

latin-panda Apr 25, 2025

lognaturel Apr 25, 2025

lognaturel Apr 25, 2025

latin-panda Apr 28, 2025

latin-panda Apr 25, 2025

latin-panda Apr 25, 2025 •

edited

Loading

latin-panda Apr 25, 2025

latin-panda commented Apr 25, 2025

lognaturel left a comment

lognaturel Apr 25, 2025

lognaturel Apr 25, 2025

alyblenkin commented Apr 25, 2025

latin-panda commented Apr 28, 2025

		@@ -38,8 +41,6 @@

		--odk-border-color: var(--p-surface-300);

		--odk-icon-size: 1.5rem;


		By following the steps above, you should minimize the diff. However, in the JSON file, you may still see changes for properties like `id`, `iconIdx`, `setId`, and `setIdx`.
		- Import the icon from `@mdi/js` in the `Icon` component.

feat(#280): adds icon component with material desing icons #384

feat(#280): adds icon component with material desing icons #384

Conversation

latin-panda commented Apr 25, 2025 • edited Loading

I have verified this PR works in these browsers (latest versions):

What else has been done to verify that this works as intended?

Why is this the best possible solution? Were any other approaches considered?

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Do we need any specific form for testing your changes? If so, please attach one.

What's changed

changeset-bot bot commented Apr 25, 2025 • edited Loading

🦋 Changeset detected

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

latin-panda Apr 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

latin-panda commented Apr 25, 2025

lognaturel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alyblenkin commented Apr 25, 2025

latin-panda commented Apr 28, 2025

latin-panda commented Apr 25, 2025 •

edited

Loading

changeset-bot bot commented Apr 25, 2025 •

edited

Loading

latin-panda Apr 25, 2025 •

edited

Loading