Skip to content
This repository has been archived by the owner on Nov 4, 2023. It is now read-only.

refactor: use modular popup for iframe popups #450

Merged
merged 21 commits into from
Oct 7, 2020
Merged

Conversation

akloeckner
Copy link
Contributor

@akloeckner akloeckner commented Oct 1, 2020

This is another refactor to make maximum use of the modular popup with the following changes

  • use modular popup for iframe, too
  • add classes to make it easier to specify size of popups and tiles inside them
  • remove old code from stand-alone iframe popup

BREAKING CHANGE: This changes some behavior:

  1. background of iframe tile is changed to transparent white, because otherwise, the background of the iframe would be awkwardly gray.
  2. iframeClasses are applied to popup-container. Not popup (including the blurred-out area around the popup). This seemed more intuitive. But I think, it could be changed to less intuitive popup to not break existing setups. (And possibly allow for styling the blurred-out area.)
  3. iframeStyles are applied to the iframe tile. Not to the popup-container. This is mainly due to the way, the popup is structured. There simply is a facility to pass styles to a tile. But there is not a facility to apply styles directly to the popup. For the user, this means that styles (e.g. borders) do not include the popup title anymore.

Fixes to these changes:

  1. set background manually if need to.
  2. fix custom.css styles to reflect new structure
  3. use iframeClasses together with custom.css and forget about iframeStyles

In summary, I would favor these changes over implementing the old structure and believe that they are not of a very big impact to the user. Especially the styles options should (in my opinion) be disfavoured in comparison to using classes options. This will make it possible for us to supply even more classes for easy customization.

@rchl
Copy link
Collaborator

rchl commented Oct 1, 2020

  1. background of iframe tile is changed to transparent, because otherwise, the background of the iframe would be awkwardly gray.

Can we set background to white by default? Transparent is possibly not the best choice as then many pages (those that don't define background color) will be transparent. Browsers use white background by default so matching that might be good.

@akloeckner
Copy link
Contributor Author

Can we set background to white by default?

Hmm... I am wondering now, why that is not an issue with my page that became gray. I'll investigate and switch to white in the process.

Copy link
Collaborator

@rchl rchl left a comment

Choose a reason for hiding this comment

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

Can we set background to white by default?

Hmm... I am wondering now, why that is not an issue with my page that became gray. I'll investigate and switch to white in the process.

Because there are various aspects to it. It depends on the theme and also on whether we are talking about the iframe tile or iframe popup.

Here is tile:
Screenshot 2020-10-01 at 20 33 47

Here is popup:
Screenshot 2020-10-01 at 20 33 57

Neither is really great as the tile became transparent and the popup background is black (or close) so the text is unreadable.
This is tested on http://elg.no page

scripts/controllers/main.js Outdated Show resolved Hide resolved
styles/main.less Outdated Show resolved Hide resolved
scripts/controllers/main.js Outdated Show resolved Hide resolved
@akloeckner
Copy link
Contributor Author

akloeckner commented Oct 1, 2020

Can we set background to white by default?

Did so. I was offset by the fact that my test iframe had an iframeStyles option, styling things white already...

(Which was actually on purpose, looking at my commit message 1fc790e introducing that change. But I agree that white is a better default. And a user can always add a style to iframeStyles or a class to iframeClasses, if they want it to be green.)

@akloeckner
Copy link
Contributor Author

I also styled the whole popup. It will allow to define, e.g., alert classes for popups, that style the whole thing red, including the blurred area. It's now consistent with the old implementation.

One thing that came back to my mind now is, that history.classes is now actually used for two elements:

  1. the popup as introduced here. (This is actually also in the docs, but I forgot about that when migrating the history popup.)
  2. the tile as the history object is merged into the generated tile object.

It should not be a big deal, I believe, because the user can always implement custom.css in a way that their classes need to be applied to a .popup or a .item. But it clearly might be a bit confusing.

scripts/controllers/main.js Outdated Show resolved Hide resolved
styles/main.less Outdated Show resolved Hide resolved
@rchl
Copy link
Collaborator

rchl commented Oct 2, 2020

Heads up: I might not be able to finish reviewing it this weekend as I'll be away.

@akloeckner
Copy link
Contributor Author

Heads up: I might not be able to finish reviewing it this weekend as I'll be away.

Fine. That means more time for my family. :-)

scripts/controllers/main.js Outdated Show resolved Hide resolved
styles/main.less Outdated Show resolved Hide resolved
styles/main.less Outdated Show resolved Hide resolved
@rchl rchl added the hacktoberfest-accepted Accept for Hacktoberfest label Oct 6, 2020
@rchl rchl merged commit bc42eea into resoai:master Oct 7, 2020
@rchl
Copy link
Collaborator

rchl commented Oct 7, 2020

Thanks!

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

Successfully merging this pull request may close these issues.

2 participants