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

Initial version of date column #1363

Merged
merged 15 commits into from
Jul 24, 2023
Merged

Initial version of date column #1363

merged 15 commits into from
Jul 24, 2023

Conversation

m-akinc
Copy link
Contributor

@m-akinc m-akinc commented Jul 19, 2023

Pull Request

🀨 Rationale

Part of #1014

πŸ‘©β€πŸ’» Implementation

This is the first, basic version of the column. It does not support any configuration. It always formats the datetime in the way that will become the default when we later add format configuration. The column derives from TableColumnTextBase.

  • component defined
  • initial Storybook stories/documentation created
  • unit tests written

πŸ§ͺ Testing

Tested in Storybook and created unit tests.

βœ… Checklist

  • I have updated the project documentation to reflect my changes or determined no changes are needed.

try {
this.text = formatter
.format(this.cellRecord.value)
.replace('\u202f', ' '); // on Chrome, the space before AM/PM is a narrow non-breaking space. For testing consistency across browsers, replace it with a regular space.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this is a buggy behavior worth overriding. Did you check from that perspective?

If the goal is just making tests easier it should be a part of the page object. I'm not sure there is a clear benefit in trying to override the details of the native date formatter to get the exact same rendered text across browsers. Is there a specific argument for that?

One argument against could be that many apps are likely to use the same formatter. So our date text will render similarly to most other web apps in Chrome.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it's a bug--just a mostly trivial difference between how each browser formats the date. It seems more targeted to doing the replacement here than in the page object. If I do it in the page object, I either have to do this substitution for all text rendered in cells, or add an esoteric new function to fetch a rendered date from a cell. I don't see any practical downsides to replacing one narrow non-breaking space with a regular space. That was my thought process, at least.

Copy link
Member

Choose a reason for hiding this comment

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

Seems like the date table column could have it's own page object

Copy link
Member

@rajsite rajsite Jul 19, 2023

Choose a reason for hiding this comment

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

edit: removed, wasn't very productive

@m-akinc m-akinc requested a review from rajsite July 20, 2023 19:38
@m-akinc m-akinc enabled auto-merge (squash) July 24, 2023 14:23
@m-akinc m-akinc merged commit 44e7d0d into main Jul 24, 2023
4 checks passed
@m-akinc m-akinc deleted the users/makinc/datetime-column branch July 24, 2023 14:47
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