Skip to content
This repository has been archived by the owner on Jul 19, 2021. It is now read-only.

Add parameters to RTE wrapper methods #284

Merged
merged 5 commits into from
Sep 18, 2017

Conversation

maximevaillancourt
Copy link
Contributor

What are you trying to accomplish with this PR?

Instead of hard-coding selector & class strings directly in the wrapper methods, we allow parameters to be passed in, which improves the scalability and reusability of these methods when creating a new theme.

Checklist

  • I have 🎩'd these changes.

Maxime Vaillancourt added 2 commits September 15, 2017 13:40
Instead of hard-coding strings in the div-wrapping rte methods,
we allow parameters to be passed to improve future scalability
and reusability of these methods throughout themes.
Instead of hard-coding strings directly in the div-wrapping rte methods,
we allow parameters to be passed to improve scalability
and reusability of these methods throughout themes.
@NathanPJF
Copy link
Contributor

@maximevaillancourt could you please update the Docs site as well to reflect these changes? https://shopify.github.io/slate/js-examples/#responsive-tables-and-videos

Info on how to update Docs: https://github.com/Shopify/slate/blob/master/CONTRIBUTING.md#documentation

@maximevaillancourt
Copy link
Contributor Author

@NathanPJF Just updated the docs, see 9718335.

@NathanPJF
Copy link
Contributor

NathanPJF commented Sep 15, 2017

@maximevaillancourt could you please add a parameters table as well as seen in the other methods on the docs site?

Screenshot of trap focus table

 

This follows the same style used in the Trap Focus section, which
explains each parameter used in the `trapFocus` method.
@maximevaillancourt
Copy link
Contributor Author

@NathanPJF Yup, added in 7b9ddaa.

| Parameters | Type | Description |
| :------------------- | :------------ | :------------ |
| `$tables` | jQuery object | `<table>` elements to be made responsive |
| `tableWrapperClass` | string | CSS class to apply on the `<div>` that will wrap each targetted `<table>` element |
Copy link
Contributor

Choose a reason for hiding this comment

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

targeted instead of targetted

| `$tables` | jQuery object | `<table>` elements to be made responsive |
| `tableWrapperClass` | string | CSS class to apply on the `<div>` that will wrap each targetted `<table>` element |
| `$iframes` | jQuery object | `<iframe>` elements to be made responsive |
| `iframeWrapperClass` | string | CSS class to apply on the `<div>` that will wrap each targetted `<iframe>` element |
Copy link
Contributor

Choose a reason for hiding this comment

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

targeted instead of targetted

Copy link
Contributor

@NathanPJF NathanPJF left a comment

Choose a reason for hiding this comment

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

Great, @maximevaillancourt ! Just two small tweaks and then feel free to merge :)

@maximevaillancourt maximevaillancourt merged commit 735ec29 into master Sep 18, 2017
@maximevaillancourt maximevaillancourt deleted the add-parameters-rte-methods branch September 18, 2017 14:28
@lock
Copy link

lock bot commented Oct 26, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Oct 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants