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.

Sign up for GitHub

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

Jump to bottom

report(redesign): remove units from table columns #8658

Merged
merged 1 commit into from
Apr 29, 2019
Merged

report(redesign): remove units from table columns #8658

merged 1 commit into from
Apr 29, 2019

Conversation

connorjclark
Copy link
Collaborator

@brendankenny
Copy link
Member

before/after screenshot?

@connorjclark
Copy link
Collaborator Author

before
image
after
image

exterkamp
exterkamp approved these changes Apr 26, 2019
View reviewed changes
Copy link
Member

@exterkamp exterkamp left a comment

Choose a reason for hiding this comment

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

LGTM. We need to sync this ASAP if we want it within a week. But the not so bad news is that the other locales will just say "X (ms)" instead of just "X" so it's not crazy critical.

patrickhulce
patrickhulce approved these changes Apr 26, 2019
View reviewed changes
Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

LGTM!

paulirish
paulirish approved these changes Apr 29, 2019
View reviewed changes
Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

i checked the translations to see if we end up with different strings for "savings" given that one means "time savings" and the other means "byte savings"... and.... the strings are different for like 20% of locales... so anyway... yeah lets keep this as is.

@paulirish paulirish merged commit 0e513a6 into master Apr 29, 2019
@paulirish paulirish deleted the rd-remove-table-column-units branch April 29, 2019 23:56
@exterkamp
Copy link
Member

LGTM. We need to sync this ASAP if we want it within a week. But the not so bad news is that the other locales will just say "X (ms)" instead of just "X" so it's not crazy critical.

Oh how naive I was. This breaks tc/ :laughing:

@patrickhulce
Copy link
Collaborator

breaks tc? How!?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@exterkamp exterkamp exterkamp approved these changes

@patrickhulce patrickhulce patrickhulce approved these changes

@paulirish paulirish paulirish approved these changes

@brendankenny brendankenny Awaiting requested review from brendankenny

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@connorjclark @brendankenny @exterkamp @patrickhulce @paulirish