Skip to content

Conversation

@Dravenshorst
Copy link
Contributor

  • Adds function to create a window from URL inside iframe or div.
  • Also adds a refresh button.

Signed-off-by: Daniel Ravenshorst daniel.ravenshorst@dealerdirect.nl

* Extends function `createWindow` with the function `fromUrl`.
* Adds option to load in div or iframe (default = div).
* Adds function `setXhrResponse` in `wm/windows.js` to load the data in a div.
* Adds refresh btn to window template.
* Adds styling for `btn.wm-refresh`.
* Adds an example to desktop with iframe and div.
* Adds compiled assets in `/dist`.

Signed-off-by: Daniel Ravenshorst <daniel.ravenshorst@dealerdirect.nl>
* Fixes merge conflict.

Signed-off-by: Daniel Ravenshorst <daniel.ravenshorst@dealerdirect.nl>
* Updates dist folder.

Signed-off-by: Daniel Ravenshorst <daniel.ravenshorst@dealerdirect.nl>
@frenck
Copy link

frenck commented Jun 18, 2018

bitmoji

@rlamana
Copy link
Owner

rlamana commented Jun 19, 2018

@Dravenshorst I still have mixed feelings about this PR. The idea of the library is to keep it as light and simple as possible and just focus on window management, not on the content of the windows. I see it as just a wrapper around content, no matter if it is a single DOM element, a complex DOM tree or an iframe. That is up to the developer of the wrapped "app"/content.

What counter arguments do you have to convince me to include it as part of ventus? ;)

…esh-btn-and-create-window-from-url

Signed-off-by: Daniel Ravenshorst <daniel.ravenshorst@dealerdirect.nl>

# Conflicts:
#	dist/ventus.min.js
* Updates `dist/` folder.
* Removes examples from desktop.
* Added example to simple.
* Removes jQuery from `window.js` as it's not needed anymore.
* Removes function `setXhrResponse`.
* Adds check on reload event. If added then refresh-button is shown.
* Removes function `fromUrl`.
* Updates README.md

Signed-off-by: Daniel Ravenshorst <daniel.ravenshorst@dealerdirect.nl>
@Dravenshorst
Copy link
Contributor Author

Dravenshorst commented Jun 21, 2018

@rlamana I understand your concerns.
The main goal of this PR is to add an refresh button.
So I have changed this PR to listen to the reload event. If the reload event is defined it will show a refresh button. The functionality of the button is up to the user an can be defined in the event callback.
I have added an working example of how I am using it in examples/simple.

Can you have a look at it again?
Thanks.

* Fixes line indenting.

Signed-off-by: Daniel Ravenshorst <daniel.ravenshorst@dealerdirect.nl>
@rlamana
Copy link
Owner

rlamana commented Jun 22, 2018

@Dravenshorst A more generic approach (aligned with the goal of this library) would be to provide a way to customize the content of the title bar. Maybe we could provide a flexible area, between the title and the buttons where the developer can attach/append a DOM element and extend the functionality of the title bar.

Your solution seems to me too coupled to the code that solves your iframe requirement, don't you think?

@rlamana rlamana closed this Mar 17, 2020
@frenck
Copy link

frenck commented Mar 17, 2020

Closing without saying anything! Best 👍
Thanks, m8!

@rlamana
Copy link
Owner

rlamana commented Mar 17, 2020

@frenck Sorry I assumed this PR was abandoned. It's been almost 2 years and I got no response to the conversation I started. Also, there are merge conflicts that haven't been fixed. I'm more than glad to reopen it if someone is going to continue with it.

@rlamana rlamana reopened this Mar 17, 2020
@frenck
Copy link

frenck commented Mar 17, 2020

I'm not involved in the company that used it anymore, so I don't mind. I just received a closing notification without any comment on this PR.

In general, I think that is not nice. At least have the decency to leave a message with something like: "Yooo m8, this PR looks stale, will close it for now. Feel free to re-open when you are ready to work on it again" or similar.

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.

3 participants