Skip to content

feat(ui5-dynamic-date-range): work in progress #10895

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

Draft
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

tsanislavgatev
Copy link
Contributor

Do not review, presentational purposes only.

@github-actions github-actions bot added the Stale label Mar 14, 2025
@github-actions github-actions bot removed the Stale label Mar 21, 2025
@github-actions github-actions bot added the Stale label May 7, 2025
@github-actions github-actions bot removed the Stale label May 10, 2025
@vladitasev
Copy link
Contributor

vladitasev commented May 12, 2025

High-level notes:

  • DynamicDateRange should not import all kinds of options directly -they should be imported in the bundle by the app instead
  • DynamicDateValue is currently a custom element, but I did not see it used anywhere as such. Probably should be just a normal JS object because this way it can be bound by apps using frameworks.
  • DynamicDateRange has no value if I choose the simple options such as Today, Tomorrow (ones without a special template).
  • Specific data type for the values property of DynamicDateValue to be determined

@vladitasev
Copy link
Contributor

  • the options files can be called just Today.ts, Tomorrow.ts etc. because the name is already prefixed in dynamic-date-range-options/ and there's no need to repeat it
  • It is possible to click Submit for the date range option while you have selected only 1 date and you get a wrong value for the component
  • When you manually edit the Input, f.e. change 1 number (let's say the day of a date) and press Enter, the value becomes "Today"
  • When Today, Tomorrow are selected, the empty values array in the value property should ideally not be there at all since it's optional

Copy link
Contributor

@vladitasev vladitasev left a comment

Choose a reason for hiding this comment

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

Looks good

Copy link
Contributor

@unazko unazko left a comment

Choose a reason for hiding this comment

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

  1. Lacking "Has Details" announcement for the list items
  2. Lacking tooltip for the navigation icon (should not be decorative)
  3. Lacking hover effects over the value help icon
  4. Value help icon should be pressed twice to open the popover
  5. Press out of the popover doesn't close the popover
  6. Initial focus should be over the selected option if such is present
  7. F4, Option + Arrow Up/Down doesn't toggle the value help
  8. The text element showcasing the selected date in the SingleDate option details doesn't get cleared after the text is erased from the input (On next open we have the previous value)
  9. If we navigate to the SingleDate option detail page and close the value help then on next value help open we'll be directly navigated to the SingleDate option page again.
  10. After navigation the focus is not getting visualised (back and in details pages)
  11. If we select a Date from the SingleDate page and then navigate to the DateRange page we'll already have a selected start date in the range, which appears unexpected. We do have similar mixing with the other options as well. For example if we select Yesterday and then open the SingleDate option we'll already have a selected date.

@tsanislavgatev tsanislavgatev requested a review from unazko May 23, 2025 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants