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

fix(alert): use v-model to update show value #721

Merged
merged 7 commits into from Jul 24, 2017

Conversation

Projects
None yet
3 participants
@alexsasharegan
Contributor

alexsasharegan commented Jul 21, 2017

Fix for issue #718

My initial work on the idea of using v-model. I haven't run my bug test on this yet because things are really busy for me.
Edit: This does clear up my bug.

Note: This may need to be tweaked as doing <b-alert show dismissible> doesn't allow the alert to be dismissed. The addition of v-model becomes necessary. It might be necessary to have an initial show either as a prop or as a data value local to alert. But if you think about it, it's fundamentally flawed to declare both show and dismissible at the same time while also making alerts reusable.

Feel free to contribute on this branch.

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Jul 21, 2017

Yeah, maybe a local copy of show that is manipulated internally and a watcher on show?

@tmorehouse tmorehouse added this to the v0.19.0 milestone Jul 21, 2017

@alexsasharegan alexsasharegan changed the title from [WIP] fix(alert): use v-model to update show value to fix(alert): use v-model to update show value Jul 22, 2017

@alexsasharegan

This comment has been minimized.

Contributor

alexsasharegan commented Jul 22, 2017

I've pushed a change to use a local copy of the v-model prop. Ready for everyone to hammer away at it!

Along for the ride are some testing helpers I've cooked up while working through functional components. Basically the expect.toHaveClass && expect.toHaveAllClasses can take a vue instance or html element. It will detect the type and defer to the proper class lookup strategy.

When you use the expect helpers wrong, they throw all kinds of useful errors to help you use them properly or debug the problem.

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Jul 22, 2017

Looks good!

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Jul 22, 2017

Yeah, the current expect builtins like .toBe(...) report vague errors and you have to resort to looking up the line number in the test to see which part it was.

@tmorehouse tmorehouse requested review from tmorehouse and mosinve Jul 24, 2017

@tmorehouse

Looks good from what I can see.

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Jul 24, 2017

Are we good to merge this into master?

@tmorehouse tmorehouse merged commit 9b380d0 into master Jul 24, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@mosinve

This comment has been minimized.

Member

mosinve commented Jul 24, 2017

Argh.. i just wanted to write, that bug at docs alert example, mentioned in #718 at last post is not gone...

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Jul 24, 2017

No? I can revert back and we can update the branch

@mosinve

This comment has been minimized.

Member

mosinve commented Jul 24, 2017

i think it should be inspected more detailed

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Jul 24, 2017

Shall I revert?

@mosinve

This comment has been minimized.

Member

mosinve commented Jul 24, 2017

Yeah, if it can be reverted.

tmorehouse added a commit that referenced this pull request Jul 24, 2017

@mosinve

This comment has been minimized.

Member

mosinve commented Jul 24, 2017

I'll see what can be done to fix it

tmorehouse added a commit that referenced this pull request Jul 24, 2017

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Jul 24, 2017

I'll create a new pull request for the branch.

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Jul 24, 2017

New PR #734 created which we can make adjustments to as needed.

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