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

Remove OK button from modals #1134

Merged
merged 1 commit into from
Mar 23, 2020
Merged

Conversation

openorclose
Copy link
Contributor

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [x] Bug fix

Fixes #1126

What is the rationale for this request?

Remove the redundant OK button from modals, only show it if the author specifies ok-title.

What changes did you make? (Give an overview)

If there is no ok-title attribute, and there is no footer slot, hide the footer.

Is there anything you'd like reviewers to focus on?

Testing instructions:
Added a test and a docs example to show the case where ok-text is set.

Proposed commit message: (wrap lines at 72 characters)

Remove OK button from modals

Let's remove the redundant OK button from modals,
and only show it if the `ok-title` attribute is present.

Copy link
Contributor

@ang-zeyu ang-zeyu left a comment

Choose a reason for hiding this comment

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

Just some nits:


if (!hasFooter && !hasOkTitle) {
// markbind doesn't show the footer by default
node.attribs['hide-footer'] = '';
Copy link
Contributor

Choose a reason for hiding this comment

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

does setting it to true erase the ="" portion in hide-footer=""? might be nice if so, same for ok-only

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, i just get this

image

consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur.
Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.
</modal>

```
</span>
<span id="output">
Copy link
Contributor

Choose a reason for hiding this comment

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

should ok-text be ok-title instead? ( the props table )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be fine as ok-text. since it's not really a title of anything.

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the redundant OK button from modals, only show it if the author specifies ok-title.

Ahh ok, thought the attribute from the user standpoint here was ok-title

Copy link
Contributor

@ang-zeyu ang-zeyu left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Contributor

@marvinchin marvinchin left a comment

Choose a reason for hiding this comment

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

LGTM

@marvinchin marvinchin added this to the v2.12.1 milestone Mar 23, 2020
@marvinchin marvinchin merged commit 407158b into MarkBind:master Mar 23, 2020
Tejas2805 added a commit to Tejas2805/markbind that referenced this pull request Mar 23, 2020
* 'master' of https://github.com/MarkBind/markbind:
  Update test files (MarkBind#1138)
  Remove OK button from modals (MarkBind#1134)
  Add start from line number functionality to code blocks (MarkBind#1115)
  Allow special tags to be self-closing (MarkBind#1101)
  Simplify baseUrl resolving process (MarkBind#1087)
  Remove redundant file writing (MarkBind#1090)
  Bump acorn from 5.7.3 to 5.7.4 (MarkBind#1121)
  Bump acorn from 7.1.0 to 7.1.1 in /src/lib/markbind (MarkBind#1120)
  Unify markdown-it parser variants (MarkBind#1056)
  Remove dynamic include feature (MarkBind#1037)
  Fix flex shrink not applying in content wrapper (MarkBind#1135)
  Escape Nunjucks for special tags (MarkBind#1049)
  Update documentation on icon slot for boxes (MarkBind#1123)
Tejas2805 added a commit to Tejas2805/markbind that referenced this pull request Mar 23, 2020
…x-pageNav

* 'master' of https://github.com/MarkBind/markbind:
  Update test files (MarkBind#1138)
  Remove OK button from modals (MarkBind#1134)
  Add start from line number functionality to code blocks (MarkBind#1115)
  Allow special tags to be self-closing (MarkBind#1101)
  Simplify baseUrl resolving process (MarkBind#1087)
  Remove redundant file writing (MarkBind#1090)
  Bump acorn from 5.7.3 to 5.7.4 (MarkBind#1121)
  Bump acorn from 7.1.0 to 7.1.1 in /src/lib/markbind (MarkBind#1120)
  Unify markdown-it parser variants (MarkBind#1056)
  Remove dynamic include feature (MarkBind#1037)
  Fix flex shrink not applying in content wrapper (MarkBind#1135)
  Escape Nunjucks for special tags (MarkBind#1049)
  Update documentation on icon slot for boxes (MarkBind#1123)
  Convert code in boxes to code block (MarkBind#1086)
marvinchin pushed a commit that referenced this pull request Apr 10, 2020
Let's remove the redundant OK button from modals,
and only show it if the `ok-title` attribute is present.
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.

Remove OK button from modals
3 participants