Features/824 redesign data pages #1133

sadiqkhoja · 2025-02-06T21:10:41Z

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

Manual verification.

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

Most of it is HTML/CSS changes. Though not specified in the release criteria, I have made the similar changes on Audits page.

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?

No

Does this change require updates to user documentation? If so, please file an issue here and include the link below.

No

Before submitting this PR, please make sure you have:

run npm run test and npm run lint and confirmed all checks still pass OR confirm CircleCI build passes
verified that any code or assets from external sources are properly credited in comments or that everything is internally sourced

…issions pages

matthew-white · 2025-03-05T15:25:31Z

src/components/audit/filters/action.vue

+  <label id="audit-filters-action" for="audit-actions" class="form-group">
+    <select id="audit-actions" ref="auditActions" class="form-control" :value="modelValue"


If the form control is in the <label>, I don't think the <label> needs for. From MDN:

you can nest the <input> directly inside the <label>, in which case the for and id attributes are not needed because the association is implicit

matthew-white · 2025-03-05T15:34:20Z

src/components/date-range-picker.vue

      :aria-disabled="disabled" v-tooltip.aria-describedby="disabledMessage"
-      :placeholder="requiredLabel(placeholder, required)" autocomplete="off"
+      :placeholder="$t('common.none')" autocomplete="off"


I don't think it's possible to use a single word for "none" for different filters, since some languages inflect "none" based on what it describes. For example, we translate submission.noSubmission and entity.noEntity separately. I think each filter probably needs its own translation for "none".

matthew-white · 2025-03-05T15:38:28Z

src/components/multiselect.vue

@@ -10,19 +10,23 @@ including this file, may be copied, modified, propagated, or distributed
 except according to the terms contained in the LICENSE file.
 -->
 <template>
-  <div ref="dropdown" class="multiselect form-group">
+  <label ref="dropdown" :for="toggleId" class="multiselect form-group">


What's the benefit of making this a <label>? Since this is the root element, and it encloses the .dropdown-menu, I'd be slightly concerned that it'd include that content in the label. The <select> has an aria-label, so I don't think a <label> element is essential here.

I wanted to make all elements clickable to open the dropdown menu, but I see what you are saying. I am removing it and just wrapping select, span for {{ label }} and span.icon in a new div with data-toggle

matthew-white · 2025-03-05T15:43:29Z

src/components/date-range-picker.vue

@@ -10,14 +10,15 @@ including this file, may be copied, modified, propagated, or distributed
 except according to the terms contained in the LICENSE file.
 -->
 <template>
-  <label class="form-group"><!-- eslint-disable-line vuejs-accessibility/label-has-for -->


The linter doesn't usually yell at us about label-has-for. I think it's doing so here because it doesn't understand that <flatpickr> is really an <input> element. As long as the <label> wraps the <input>, we shouldn't need for.

I have removed the for attribute and add a rule in eslintrc so that it recognizes flatpickr as a control component.

matthew-white · 2025-03-05T15:53:52Z

src/components/multiselect.vue

    <!-- Specifying @mousedown.prevent so that clicking the select element does
    not show a menu with the placeholder option. This approach seems to work
    across browsers. -->
    <select :id="toggleId" ref="toggle" class="form-control"
+      :class="{ none: modelValue?.length === options?.length }"


Suggested change

:class="{ none: modelValue?.length === options?.length }"

:class="{ none: defaultToAll && modelValue?.length === options?.length }"

If defaultToAll is false (the default), then the user can actually uncheck all options. In that case, modelValue.length === 0. All and none are only the same if defaultToAll is true.

matthew-white · 2025-03-05T15:55:25Z

src/components/multiselect.vue

@@ -133,6 +137,10 @@ const props = defineProps({
    type: String,
    required: true
  },
+  hideLabel: {


This section of props is about customizing text, so I'd put this prop somewhere else. Maybe nearby the disabled prop?

matthew-white · 2025-03-05T16:02:02Z

src/components/dataset/entities.vue

+            @analyze="analyze.show()"/>
+          <entity-download-button
+            :aria-disabled="deleted"
+            v-tooltip.aria-describedby="deleted ? $t('downloadDisabled') : null"/>


I think the message for downloadDisabled needs to be moved from EntityList. You can use @transifexKey to prevent the message from being retranslated after it's moved.

matthew-white · 2025-03-05T16:09:34Z

src/components/entity/download-button.vue

 import { apiPaths } from '../../util/request';
-import { useI18nUtils } from '../../util/i18n';
-import { useRequestData } from '../../request-data';

 const props = defineProps({


It doesn't look like DatasetEntities passes an odataFilter prop to this component anymore. If the component doesn't use the prop anymore, it could remove the prop. But it sounds to me from the release criteria that the component does need the prop:

Except if I have an active filter then above the checkboxes I should see a dropdown with two options:

Download # submissions matching the filter

Download all # submissions

matthew-white · 2025-03-05T16:20:20Z

src/components/form/submissions.vue

+          <odata-data-access :analyze-disabled="analyzeDisabled"
+            :analyze-disabled-message="analyzeDisabledMessage"
+            @analyze="analyzeModal.show()"/>
+          <submission-download-button :form-version="form"


I'm wondering if we could leave the download button in SubmissionList, but make it teleport here. Then the button would still be able to communicate with the download modal easily. Also, we wouldn't need to copy the downloadDisabled message here.

Just an idea!

matthew-white · 2025-03-05T16:44:04Z

One other thing that the release criteria mention is:

Except in the popup, I should see the “Other” renamed to “Other (API)”

sadiqkhoja force-pushed the features/824-redesign-data-pages branch from 5ad7c12 to 0e82126 Compare February 6, 2025 21:17

sadiqkhoja force-pushed the features/824-redesign-data-pages branch 3 times, most recently from fc7f618 to 3cf4135 Compare February 28, 2025 20:40

Feature getodk/central#824: Redesign top section of Entities and Subm…

3e59dc2

…issions pages

sadiqkhoja force-pushed the features/824-redesign-data-pages branch from 3cf4135 to 3e59dc2 Compare February 28, 2025 20:57

sadiqkhoja marked this pull request as ready for review February 28, 2025 20:57

sadiqkhoja requested a review from matthew-white February 28, 2025 21:00

matthew-white reviewed Mar 5, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Features/824 redesign data pages #1133

Features/824 redesign data pages #1133

sadiqkhoja commented Feb 6, 2025 •

edited

Loading

matthew-white Mar 5, 2025

matthew-white Mar 5, 2025

matthew-white Mar 5, 2025

sadiqkhoja Mar 8, 2025

matthew-white Mar 5, 2025

sadiqkhoja Mar 8, 2025

matthew-white Mar 5, 2025

matthew-white Mar 5, 2025

matthew-white Mar 5, 2025

matthew-white Mar 5, 2025

matthew-white Mar 5, 2025

matthew-white commented Mar 5, 2025

		<label id="audit-filters-action" for="audit-actions" class="form-group">
		<select id="audit-actions" ref="auditActions" class="form-control" :value="modelValue"

	:class="{ none: modelValue?.length === options?.length }"
	:class="{ none: defaultToAll && modelValue?.length === options?.length }"

Features/824 redesign data pages #1133

Are you sure you want to change the base?

Features/824 redesign data pages #1133

Conversation

sadiqkhoja commented Feb 6, 2025 • edited Loading

What 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?

Does this change require updates to user documentation? If so, please file an issue here and include the link below.

Before submitting this PR, please make sure you have:

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

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

matthew-white commented Mar 5, 2025

sadiqkhoja commented Feb 6, 2025 •

edited

Loading