Feature/ember upgrade #153

josex2r · 2021-11-24T07:59:34Z

No description provided.

BRAKING CHANGES: upgrade Ember version

codecov · 2021-11-24T08:13:42Z

Codecov Report

Merging #153 (45448aa) into master (7791d42) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #153   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            6         7    +1     
  Lines           82       100   +18     
=========================================
+ Hits            82       100   +18

Impacted Files	Coverage Δ
addon/components/modal-container/index.js	`100.00% <100.00%> (ø)`
addon/components/modal-wrapper/index.js	`100.00% <100.00%> (ø)`
addon/components/modal/index.js	`100.00% <100.00%> (ø)`
addon/helpers/open-modal.js	`100.00% <100.00%> (ø)`
addon/models/modal.js	`100.00% <100.00%> (ø)`
addon/services/modal.js	`100.00% <100.00%> (ø)`
addon/utils/css-transitions/has-transitions.js	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 65b4bda...45448aa. Read the comment docs.

# Conflicts: # README.md # addon/components/modal.js # addon/services/modal.js

BRAKING CHANGE: modal component extends from Glimmer

…the name

BREAKING CHANGE: promise is fulfilled when the modal animation ends

adrigzr · 2021-12-14T15:56:29Z

addon/models/modal.js


-	init() {
-		super.init(...arguments);
+  get componentName() {


This should be inside modal-wrapper and not in the model.

xe21500 · 2021-12-14T15:58:48Z

addon/helpers/open-modal.js

+    return () => {
+      this.modal
+        .open(modalName, options)
+        .then(actions.onDone, actions.onFail, 'open-modal helper');


async / await here?

xe21500 · 2021-12-15T06:58:45Z

addon/services/modal.js

+    this.trigger('open', model);
+
+    model.promise
+      .catch(() => {})


I'd try...catch here instead

SergeAstapov · 2021-12-21T18:49:41Z

addon/components/modal-wrapper/index.hbs

@@ -0,0 +1,3 @@
+<div role="dialog" data-id={{this.dataId}} data-modal-show={{this.dataModalShow}} {{did-insert this.onDidInsert}}>
+  <@model.componentName @model={{@model}} @changeVisibility={{this.changeVisibility}} @element={{this.element}} />


@josex2r I may be wrong, but this should not work per emberjs/ember.js#17744

I assume, for embroider compatibility we should use ensure-safe-component helper provided by @embroider/util package like so

{{#let (ensure-safe-component (component @model.componentName)) as |Modal|}} <Modal @model={{@model}} @changeVisibility={{this.changeVisibility}} @element={{this.element}} /> {{/let}}

see docs https://github.com/embroider-build/embroider/tree/master/packages/util#the-utilities

my bad, didn't see it's actually being used in model.js.
IMO it's better to do component/template manipulations in the hbs file

Nice! I didn't read that the helper exists XD

SergeAstapov · 2021-12-21T18:52:32Z

addon/models/modal.js


-	@tracked _deferred;
+  @tracked _deferred = defer();


_deferred does not change during object lifecycle, maybe we don't need to use @tracked here?

SergeAstapov · 2021-12-21T18:57:43Z

addon/components/modal/index.js

+
+  @action
+  resolve(data) {
+    this._fullfillmentFn = () => this.model.resolve(data);


minor, this may be implemented using destroyable like so

registerDestructor(this, () => this.model.resolve(data));

not a big deal at all, but IMO it's easier to follow the code flow

SergeAstapov · 2021-12-21T19:05:04Z

addon/components/modal/index.js

+
+  @isNotDestroyed
+  _safeDidOpen() {
+    this.didOpen && this.didOpen();


should this be this.args.didOpen?

No, this function is something like a component life-cycle hook, to notify when the modal is visible.

Was created to extend the modal class and prevent checking if a CSS transition exists and attaching to it using more callbacks:

// components/modal-foo.js class ModalFoo extends ModalComponent { didOpen() { // Something to do here... } }

@josex2r got it! Thanks for explanation!

SergeAstapov · 2021-12-21T19:11:48Z

.npmignore

-/.nycrc.json
-/.releaserc
-/greenkeeper.json
-/jsconfig.json


jsconfig.json probably should be kept as it should not be published to npm

SergeAstapov · 2021-12-21T19:14:36Z

@josex2r Thanks for working on this!

# Conflicts: # addon/components/modal.js # addon/services/modal.js # package.json # tests/acceptance/modal-component-test.js # tests/dummy/app/components/modal-custom-modal/index.hbs # tests/dummy/app/styles/app.css # tests/integration/services/modal-test.js # yarn.lock

SergeAstapov · 2022-05-23T19:53:43Z

@josex2r @xe21500 @adrigzr is there something I can help to push this over the line and get released? I'm happy to help maintaining this addon.

If that could help, I can help and split changes proposed in this PR into smaller PRs that are easier to review - if that can help expedite things.

In addition to general Ember upgrades and transition to Glimmer component, this addon also great candidate for v2 format conversion

adrigzr · 2022-05-24T06:08:35Z

@etarancon, could you help @SergeAstapov with this? 😄

SergeAstapov · 2022-11-29T11:34:30Z

@josex2r @etarancon @adrigzr would you like to add me as maintainer of the addon as I can get this, general addon upgrades/maintenance and v2 format conversion done

next-joserepresa and others added 10 commits November 23, 2021 09:01

feat: wait until modal is destroyed

d5bec8f

docs: update README

e1ce8f5

test: fix coverage

d4d6687

feat: upgrade Ember version v3.21.2...v3.28.4

d575196

BRAKING CHANGES: upgrade Ember version

style: prettier

32f9fb8

style: test prettier rules

49a8d69

feat: bound resolve and reject to be called as actions

d9b5963

refactor(db-254623): more Octane upgrades

03e8fef

ci(db-254623): github actions and upgrade husky hooks

b813884

ci(db-254623): upgrade husky version

d1f5fae

josex2r added 6 commits November 25, 2021 17:09

feat: remove deprecations by upgrading babel

32a1e96

Merge remote-tracking branch 'origin/master' into feature/ember-upgrade

604a3cd

# Conflicts: # README.md # addon/components/modal.js # addon/services/modal.js

test: remove runloop functions

eda7cfa

feat: upgrade ember-auto-import to v2

c16d5fd

feat(octane): migrate to Glimmer components

f6d262a

BRAKING CHANGE: modal component extends from Glimmer

feat(octane): remove deprecation by using component class instead of …

c9be982

…the name

josex2r mentioned this pull request Dec 1, 2021

Glimmer components support #104

Open

feat: upgrade ember-task-scheduler

1673529

josex2r force-pushed the feature/ember-upgrade branch from 286ffa5 to 1673529 Compare December 1, 2021 10:56

josex2r added 5 commits December 1, 2021 12:03

feat: add test-waiters to deps to work with ember-embroider-safe

fa52cdb

feat: wait close animation to change promise state

7b0cd04

BREAKING CHANGE: promise is fulfilled when the modal animation ends

feat: move test-waiters to deps

1377735

test: do not wait for click

fe36e11

feat: remove ember-task-scheduler

9a64965

adrigzr reviewed Dec 14, 2021

View reviewed changes

xe21500 reviewed Dec 14, 2021

View reviewed changes

xe21500 reviewed Dec 15, 2021

View reviewed changes

addon/services/modal.js Outdated

this.trigger('open', model);

model.promise

.catch(() => {})

Copy link

xe21500 Dec 15, 2021

Choose a reason for hiding this comment

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

I'd try...catch here instead

xe21500 previously approved these changes Dec 15, 2021

View reviewed changes

SergeAstapov reviewed Dec 21, 2021

View reviewed changes

josex2r added 3 commits December 23, 2021 11:50

fix: update DOM when closing modal via service

31d319b

refactor: add some PR #153 suggestions

4ebec8f

josex2r dismissed xe21500’s stale review via 45448aa December 23, 2021 11:12

eumendoza approved these changes Nov 16, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/ember upgrade #153

Feature/ember upgrade #153

josex2r commented Nov 24, 2021

codecov bot commented Nov 24, 2021 •

edited

Loading

adrigzr Dec 14, 2021

xe21500 Dec 14, 2021

xe21500 Dec 15, 2021

SergeAstapov Dec 21, 2021

SergeAstapov Dec 21, 2021 •

edited

Loading

josex2r Dec 23, 2021

SergeAstapov Dec 21, 2021

SergeAstapov Dec 21, 2021

SergeAstapov Dec 21, 2021

josex2r Dec 23, 2021

SergeAstapov Dec 23, 2021

SergeAstapov Dec 21, 2021

SergeAstapov commented Dec 21, 2021

SergeAstapov commented May 23, 2022 •

edited

Loading

adrigzr commented May 24, 2022

SergeAstapov commented Nov 29, 2022

		@@ -0,0 +1,3 @@
		<div role="dialog" data-id={{this.dataId}} data-modal-show={{this.dataModalShow}} {{did-insert this.onDidInsert}}>
		<@model.componentName @model={{@model}} @changeVisibility={{this.changeVisibility}} @element={{this.element}} />

Feature/ember upgrade #153

Are you sure you want to change the base?

Feature/ember upgrade #153

Conversation

josex2r commented Nov 24, 2021

codecov bot commented Nov 24, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergeAstapov Dec 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergeAstapov commented Dec 21, 2021

SergeAstapov commented May 23, 2022 • edited Loading

adrigzr commented May 24, 2022

SergeAstapov commented Nov 29, 2022

codecov bot commented Nov 24, 2021 •

edited

Loading

SergeAstapov Dec 21, 2021 •

edited

Loading

SergeAstapov commented May 23, 2022 •

edited

Loading