Skip to content
This repository has been archived by the owner on Oct 20, 2022. It is now read-only.

fix: add ability to show caption on mobile or desktop only #491

Merged
merged 6 commits into from
Dec 21, 2018

Conversation

mjmonline
Copy link
Contributor

No description provided.

@coveralls
Copy link

coveralls commented Dec 21, 2018

Coverage Status

Coverage increased (+0.005%) to 83.51% when pulling 46d6797 on fix/table-caption-visibility into 9416c05 on master.

Copy link
Contributor

@JacobBlomgren JacobBlomgren 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, but the api change and its implications needs to be discussed!

KitsuneKelso
KitsuneKelso previously approved these changes Dec 21, 2018
Copy link
Member

@iamstarkov iamstarkov left a comment

Choose a reason for hiding this comment

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

I agree @JacobBlomgren. if feature landed in the public api surface—it stays until next major release.

you can mark it as deprecated.

you can make it backwards compatible.

you can introduce new ones and overshadow the old one, but in backwards compatible manner.

you should do a console warning if feature is deprecated.

you should make it easy to discover and remove.

you CANNOT remove it without releasing new major version

@mjmonline
Copy link
Contributor Author

@iamstarkov @JacobBlomgren please check again

@jesperwiner
Copy link
Contributor

@JacobBlomgren how do we mark stuff as experimental ?
After the latest adjustments it's not breaking any more so approving it.
Can be that we redo this component once all the list pages settled in and we deprecate this one then,

Copy link
Contributor

@JacobBlomgren JacobBlomgren left a comment

Choose a reason for hiding this comment

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

👏

@mjmonline mjmonline dismissed iamstarkov’s stale review December 21, 2018 11:03

problem solved, need to merge

@mjmonline mjmonline merged commit a091572 into master Dec 21, 2018
@mjmonline mjmonline deleted the fix/table-caption-visibility branch December 21, 2018 11:04
@JacobBlomgren
Copy link
Contributor

We discussed it one more round and decided to not mark stuff as experimental for now but rather iron experimental components out internally before adding them to the ui kit.

What would be nice though is to put all deprecated components at the bottom of the github.io page eventually, since we have started to deprecate a lot (which is good).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants