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

switcher as web component #395

Closed
wants to merge 16 commits into from
Closed

switcher as web component #395

wants to merge 16 commits into from

Conversation

dgrammatiko
Copy link
Contributor

Pull Request for Issue # .

Summary of Changes

Switcher is now a true web component.

Why?

  • Truly decouple css of the component from any css framework
  • Allow the component to disconnect/reconnect without breaking (eg try resizing the article edit and you'll figure out what's fixed there)
  • support for web components is looking great now (Chrome, Opera, Safari, FF(next ver)) only MS is missing, but should come before J4
  • Prepare for the future, eg support all the frameworks out there: check https://custom-elements-everywhere.com

Testing Instructions

@ciar4n can you check that everything is ok here (eg edit an article and check that the resize is working ok now and also that save is not messed up)

Expected result

Not broken

Actual result

Broken

Documentation Changes Required

@wilsonge you'll need to merge this

@ciar4n
Copy link
Contributor

ciar4n commented Apr 30, 2018

Very nice. I am unable to find any issues. Working well with shadow dom on Chrome. Fallback back to CE also working (Firefox).

Added the label also on the spoken text so for ex. you will hear now:
(off, featured no, switch) instead of (off, no, switch).
the disconect part was totally foobar
@chmst
Copy link
Contributor

chmst commented May 1, 2018

The switcher works fine but I suggest to discuss if we need a visible label?
I think if there is no visible label at all, everyone understands what the switcher shows and what to do. Green (or dark for colorblind) = on. Light = off. Click on the switcher to change th state.

Now the switcher for example shows green = yes. The label says "no".
Or the switcher is muted, but the label says "yes". This is confusing.

Alternatives are:

  • The text additionally is visible on the switcher itself, but this makes Problems with translated text.

  • Labels left and right: "yes" - buttons -"no".
    This is correct and everyone understands (but is is a challenge for designers) because the swichers then are not in line with input fields.
    .

@dgrammatiko
Copy link
Contributor Author

but this makes Problems with translated text.

That shouldn't be a problem as we're using properly translatable strings there

but is is a challenge for designers

that will make a lot of people angry

@brianteeman
Copy link
Contributor

Using colour alone to signify any meaning will be an a11y fail. If you can't see colours etc then it's a problem. The golden rule is always to have at least two visual indicators

@chmst
Copy link
Contributor

chmst commented May 2, 2018

@brianteeman in this case, the different intensity of colour could be sufficient. Because the colour has no meaning, a switcher could be blue or pink as well. The colour of the muted buttons or the label is an a11y issue, a contrast ratio of about 1.5 is too low. But this is easily changed in the css so I did not mention it.

@dgrammatiko the difficulty is that longer texts ar not readable on switchers. Have a look here: https://issues.joomla.org/tracker/joomla-cms/add - to understand the "view mode" switcher you have to inspect the source code - depending on the active language.

@dgrammatiko
Copy link
Contributor Author

Reminder here, implement a solution (eg attribute inverted) for: joomla/joomla-cms#20314

@Bakual
Copy link
Contributor

Bakual commented May 8, 2018

I agree that the labels are confusing as it is.

I also think colors will not be enough. Especially when it comes to parameters where "enabled" is where you actually turn something off. We have this in J3 in several places where the button colors are inverted (eg red for 1 and green for 0). The most known example being the "Site Offline" parameter.
With the switcher, it will be green when the site is set offline, it should be red.

@chmst
Copy link
Contributor

chmst commented May 8, 2018

@Bakual green/red or other colours are an a11y issue. But your example shows very well why colours are wrong on switchers.
See for example the issue tracker.
switcher
If the label would be "Show Help?" Then the swicher would change the state on = yes/off = no. No red/green or green/blue but different intensity of a colour - this is correct in the implementation by @dgrammatiko

Or View Mode: Helpmode - Switcher - Professional mode

@dgrammatiko
Copy link
Contributor Author

Since people were unhappy with the encapsulation here is another approach:
screen shot 2018-05-21 at 23 28 25

PROS

  • Still true web component
  • uses regular file.css, lazy loaded (only once)
  • overrides still work (eg /templates/mything/css...)

Cons

  • css and js paths are predefined and correlated ( eg something/css/file.css and something/js/file.js)

@ciar4n @wilsonge

@wilsonge
Copy link
Contributor

wilsonge commented Jun 28, 2018

So after discussion in skype this is approved pending the implementation of CSS variables (and a issue in the tracker to discuss ie11 effects)

@@ -5,6 +5,16 @@
SPACE: 32,
};

const template = document.createElement('template');
const path = document.currentScript.src;
template.innerHTML = `<link href="${path.replace('/js/', '/css/').replace('.js', '.css').replace('-es5', '')}" rel="stylesheet" type="text/css"></link>
Copy link
Member

Choose a reason for hiding this comment

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

type="text/css" << not needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unfortunately document.currentScript doesn't work with IE, so we have to encapsulate the css here. To ease the pain for newcomers we will also provide a PWA where someone will be able to tweak the colours or the scss and get an installable plugin that will create overrides to the current template and then self destruct. (or something like that...)

Copy link
Member

Choose a reason for hiding this comment

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

Im simply referring to the type attribute which is not required in this case when using HTML5

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

Successfully merging this pull request may close these issues.

None yet

7 participants