Opened 20 months ago

Closed 20 months ago

Last modified 20 months ago

#6487 closed change (fixed)

IOElement as base component for ABP UI

Reported by: agiammarchi Assignee:
Priority: P3 Milestone:
Module: User-Interface Keywords:
Cc: greiner, saroyanm Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29723623

Description

Background

We have been discussing [1] a way to bring ABP UI Components into our projects, to simplify creation of new components, test those in isolation, and the bare minimal features we want per each component.

[1] https://docs.google.com/document/d/1SH5EoFY6Cwo0xDz8LwGUSGTW7QMDXoTfqhGJ8Z7-IOQ/edit

What to change

We have agreed on few points regarding our needs and approach, summarized in here:

  • a Custom Elements based approach
  • a declarative way to define each custom element content
  • a simplified approach, when/where possible, for our i18n content

As nice to have, I think we don't want to bloat too much our code base and use technologies or polyfills that are already available or known to be available, as Web standards, soon.

Change History (10)

comment:1 Changed 20 months ago by agiammarchi

  • Review URL(s) modified (diff)

comment:2 Changed 20 months ago by asmirnov

Wrong component set for the task

comment:3 Changed 20 months ago by agiammarchi

Apologies @asmirnov I'm not sure I understand your comment. Anything I should change/do?

comment:4 Changed 20 months ago by greiner

  • Component changed from Adblock-Plus-for-Chromium to User-Interface

@asmirnov Is Adblock-Plus-for-Chromium about Orca? Because I don't see it documented anywhere and it doesn't show up on adblockplus.org/modules either so I must admit that I'm also a bit confused.

@agiammarchi I assume you meant Platform which contains all the platform-specific code related to web extensions. But I'm not sure why this ticket would concern them so I've set the ticket's module to User-interface now. Feel free to change it again if you disagree.

comment:5 Changed 20 months ago by agiammarchi

@greiner thanks. I wasn't sure myself because this is not strictly UI related but then again, it's under our repo folder so I guess the best module is UI.

comment:6 Changed 20 months ago by greiner

  • Priority changed from Unknown to P3
  • Ready set

comment:7 Changed 20 months ago by agiammarchi

  • Resolution set to fixed
  • Status changed from new to closed

comment:8 Changed 20 months ago by abpbot

A commit referencing this issue has landed:
Issue 6487 - IOElement as base component for ABP UI

comment:9 follow-up: Changed 20 months ago by Ross

This has been done and the files are included in the builds without any adverse effects.

At the moment, the test in io-element.html does not appear to work after running npm test due to: Uncaught reference error: require is not defined at io-element.js:5

comment:10 in reply to: ↑ 9 Changed 20 months ago by agiammarchi

Replying to Ross:

This has been done and the files are included in the builds without any adverse effects.

At the moment, the test in io-element.html does not appear to work after running npm test due to: Uncaught reference error: require is not defined at io-element.js:5

Ross, you cannot test io-element.html directly, you need to bundle the test and then eventually check smoke/io-element.html as described in the README.

TL;DR test folder contains test files, but to test them via a browser you inevitably need to bundle them because these are all modules and there's no require in the browser (but require is supported by any bundler).

Feel free to ignore.

Note: See TracTickets for help on using tickets.