Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(text randomizer): implement text randomizer and toggle between wheel #14

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

imballinst
Copy link
Contributor

@imballinst imballinst commented Nov 18, 2021

Closes #10. This PR contains:

  • Implementation to text randomizer component: TextBlinkRandomizer
  • Implementation to tabs to toggle between wheel and text randomizer
  • Slight tweak to fetching order: instead of fetching at the end, we fetch as the randomizer starts, so that there will be no delay after the choice is picked

Signed-off-by: Try Ajitiono <[email protected]>
Signed-off-by: Try Ajitiono <[email protected]>
Signed-off-by: Try Ajitiono <[email protected]>
@vercel
Copy link

vercel bot commented Nov 18, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/ans4175/random-gofood/9sj5fCsT2sSPyvngsK5H582VQLPM
✅ Preview: https://random-gofood-git-fork-imballinst-10-tabs-randomizer-ans4175.vercel.app

[Deployment for c8b8b9f failed]

@imballinst
Copy link
Contributor Author

@ans-4175 ada ide ngga untuk ngedengerin event change di tabsnya? Tidak terlihat ada propsnya di docs ini: https://github.com/rough-stuff/wired-elements/blob/master/docs/wired-tabs.md#wiredtabs-properties

Terus saya entah kenapa dapet eror ini:

image

padahal di Codesandbox seperti aman: https://codesandbox.io/s/react-wrapper-for-wired-elements-forked-fef11?file=/src/index.js

@ans-4175
Copy link
Owner

hf.fillPolygon

Ini kemarin aku kena isu ini di Roulette, bukan di wired element kalau tak salah.

@ans-4175
Copy link
Owner

listen event tab

https://github.com/rough-stuff/wired-elements/blob/master/src/wired-tabs.ts
Disini dia ga ada fireEvent euy, emang dia ga ngebuang sesuatu, bagaimana kalau itu dipasang ref dan baca perubahan nilai property selected-nya di wiredTabs

src/App.js Show resolved Hide resolved
@imballinst
Copy link
Contributor Author

listen event tab

https://github.com/rough-stuff/wired-elements/blob/master/src/wired-tabs.ts Disini dia ga ada fireEvent euy, emang dia ga ngebuang sesuatu, bagaimana kalau itu dipasang ref dan baca perubahan nilai property selected-nya di wiredTabs

OK, ini bisa dicoba

src/App.js Outdated
<p>Loading merchants data...</p>
) : isError || posError ? (
<p>Error: {isError ? error.message : posError.message}</p>
) : (
<>
<section>
{wheelData && (
<>
<WiredTabs selected="wheel">
Copy link
Owner

Choose a reason for hiding this comment

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

nah ini coba dipasang property selected-nya kalau berubah gitu

Copy link
Contributor Author

Choose a reason for hiding this comment

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

o ternyata dia kalau hasil build dengan NODE_ENV=production ngga eror ya? Cuma di lokal aja erornya... menarik

Copy link
Owner

Choose a reason for hiding this comment

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

error yang selected?
Aku belum nyambung

Copy link
Contributor Author

Choose a reason for hiding this comment

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

iyak, ini kalau di lokal aku, dia bakal ngecrash

tapi kalau di https://random-gofood-git-fork-imballinst-10-tabs-randomizer-ans4175.vercel.app/, dia nggak nge-crash, kenapa yak? padahal kodenya sama

Copy link
Owner

Choose a reason for hiding this comment

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

kalau aku dulu, itu masalah si pas yarn start-nya, kl dijalanin ulang lagi tetap masalah?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

iyak, dijalanin ulang tetep masalah. setelah saya liat lagi ternyata dia kalau di prod tetep ada erornya yang sama, tapi ngga bikin crash:

image

src/App.js Outdated Show resolved Hide resolved
@imballinst
Copy link
Contributor Author

@ans-4175 saya isi App.js cuma ini masih ngecrash:

import { WiredTabs, WiredTab } from 'wired-elements-react';

import './App.css';

function App() {
  return (
    <WiredTabs>
      <WiredTab name="Wheel">test</WiredTab>
      <WiredTab name="Text">test2</WiredTab>
    </WiredTabs>
  );
}

export default App;

saya coba nuke terus install ulang node_modules deh

@imballinst
Copy link
Contributor Author

wah, kurang paham, saya download file Sandbox https://codesandbox.io/s/react-wrapper-for-wired-elements-forked-fef11?file=/src/index.js, install, lalu jalanin, tetep crash 😅 mungkin harus dibawa tidur dulu baru bisa jalan

@ans-4175 kalau sempat, boleh tolong coba jalanin PR ini di lokal nggak? atau download lalu jalanin Sandbox ☝️ di lokal? takutnya ada masalah dependency yang aneh-aneh kayak apollographql/apollo-link-state#187

@imballinst
Copy link
Contributor Author

imballinst commented Nov 18, 2021

Oh, kayaknya ini penyebabnya: rough-stuff/rough@24fd61d#diff-054c66f0fb4838b0ff226ca4dea0ebcc007364ff1baf314d788a8015cbac108a

Dia ngeresolve packagenya ke package versi yang terbaru, yang mana fillPolygon udah berubah jadi fillPolygons. Mungkin kita bisa kasih resolutions di package.json sehingga dia selalu ngarah ke versi sebelumnya

EDIT: hore bisa

image

@ans-4175
Copy link
Owner

image

Iyah aku coba di codespaces juga error

@ans-4175
Copy link
Owner

Dia ngeresolve packagenya ke package versi yang terbaru

Oh oke, fixed?

@imballinst
Copy link
Contributor Author

Dia ngeresolve packagenya ke package versi yang terbaru

Oh oke, fixed?

@ans-4175 iyak, fixed. Saya submit bugnya di rough-stuff/wired-elements#179. Nanti saya follow-up di sini pakai Yarn module resolutions ke "roughjs": "4.4" untuk sementara waktu.

Signed-off-by: Try Ajitiono <[email protected]>
@imballinst
Copy link
Contributor Author

imballinst commented Nov 19, 2021

@ans-4175 saya boleh pake https://www.npmjs.com/package/patch-package aja nggak? Kayaknya lebih straightforward daripada kalau harus nambahin ref lalu listen childrennya... mungkin kalau mau nanti patchnya bisa dikontribusiin juga ke wired-elements dan wired-elements-react

wired-elements-react

diff --git a/node_modules/wired-elements-react/lib/WiredTabs.d.ts b/node_modules/wired-elements-react/lib/WiredTabs.d.ts
index cbafd3d..c37934b 100644
--- a/node_modules/wired-elements-react/lib/WiredTabs.d.ts
+++ b/node_modules/wired-elements-react/lib/WiredTabs.d.ts
@@ -1,5 +1,7 @@
 import * as React from 'react';
 import { WiredTabs as CE } from 'wired-elements/lib/wired-tabs.js';
-export declare const WiredTabs: React.ForwardRefExoticComponent<Partial<CE> & {} & {
+export declare const WiredTabs: React.ForwardRefExoticComponent<Partial<CE> & {
+    onselected?: ((e: Event) => unknown) | undefined;
+} & {
     children?: React.ReactNode;
 } & React.RefAttributes<unknown>>;
diff --git a/node_modules/wired-elements-react/lib/WiredTabs.js b/node_modules/wired-elements-react/lib/WiredTabs.js
index 6c93459..461610a 100644
--- a/node_modules/wired-elements-react/lib/WiredTabs.js
+++ b/node_modules/wired-elements-react/lib/WiredTabs.js
@@ -1,4 +1,6 @@
 import * as React from 'react';
 import { createComponent } from '@lit-labs/react';
 import { WiredTabs as CE } from 'wired-elements/lib/wired-tabs.js';
-export const WiredTabs = createComponent(React, 'wired-tabs', CE);
+export const WiredTabs = createComponent(React, 'wired-tabs', CE, {
+  onselected: 'selected'
+});

wired-elements

diff --git a/node_modules/wired-elements/lib/wired-lib.js b/node_modules/wired-elements/lib/wired-lib.js
index 5b1d153..9d3ec39 100644
--- a/node_modules/wired-elements/lib/wired-lib.js
+++ b/node_modules/wired-elements/lib/wired-lib.js
@@ -90,7 +90,7 @@ export function ellipse(parent, x, y, width, height, seed) {
 }
 export function hachureFill(points, seed) {
     const hf = new ZigZagFiller(fillHelper);
-    const ops = hf.fillPolygon(points, options(seed));
+    const ops = hf.fillPolygons([points], options(seed));
     return createPathNode(ops, null);
 }
 export function hachureEllipseFill(cx, cy, width, height, seed) {
diff --git a/node_modules/wired-elements/lib/wired-tabs.js b/node_modules/wired-elements/lib/wired-tabs.js
index f9dfb28..58c8def 100644
--- a/node_modules/wired-elements/lib/wired-tabs.js
+++ b/node_modules/wired-elements/lib/wired-tabs.js
@@ -7,7 +7,7 @@ var __decorate = (this && this.__decorate) || function (decorators, target, key,
 var __metadata = (this && this.__metadata) || function (k, v) {
     if (typeof Reflect === "object" && typeof Reflect.metadata === "function") return Reflect.metadata(k, v);
 };
-import { BaseCSS } from './wired-base';
+import { fireEvent, BaseCSS } from './wired-base';
 import { css, html, LitElement } from 'lit';
 import { customElement, property, query } from 'lit/decorators.js';
 let WiredTabs = class WiredTabs extends LitElement {
@@ -149,6 +149,7 @@ let WiredTabs = class WiredTabs extends LitElement {
                 index--;
             }
             this.selected = list[index].name || '';
+            this.fireSelected();
         }
     }
     selectNext() {
@@ -171,8 +172,12 @@ let WiredTabs = class WiredTabs extends LitElement {
                 index++;
             }
             this.selected = list[index].name || '';
+            this.fireSelected();
         }
     }
+    fireSelected() {
+        fireEvent(this, 'selected', { selected: this.selected });
+    }
 };
 __decorate([
     property({ type: String }),

@ans-4175
Copy link
Owner

ans-4175 commented Nov 19, 2021 via email

@imballinst
Copy link
Contributor Author

@ans-4175 kira-kira bagusan mana? yang pertama atau yang kedua?

image

image

yang pertama yang bawaan, yang kedua yang saya tweak. kok saya ngerasanya ada "border" di sekitar wheel itu kurang cocok ya? soalnya dia udah di atas elemen wired anyway... apakah elemen wired di atas elemen wired itu design language yang masuk?

@imballinst
Copy link
Contributor Author

Aku belum pernah pakai
Itu gimana ya?
Possible aja ya

@ans-4175 jadi kita nge-update node_modules, nanti si patch-package ini bakal nge-output diff dari file-file yang kita update. setiap kali kita pakai command yarn, dia bakal re-apply diff-diff tersebut, jadi hasilnya konsisten

@ans-4175
Copy link
Owner

ans-4175 commented Nov 19, 2021 via email

@imballinst
Copy link
Contributor Author

Kita ngunci di paketnya gitu ya terus bikin diff
Boleh deh cobain
Aku ga terlalu kebayang ji

iyak, betul 💯

Anyway, iyah tanpa border lebih masuk pas kayaknya
Itu pakai wired card buatnya? Kok aneh ya

kayaknya si tabs ini di-desain untuk berada di "tempat kosong" (bukan di atas elemen wired)... soalnya bawaan dari WiredTabnya demikian, seperti di showcasenya

image

Signed-off-by: Try Ajitiono <[email protected]>
Signed-off-by: Try Ajitiono <[email protected]>
Signed-off-by: Try Ajitiono <[email protected]>
Comment on lines +108 to +110
+ fireSelected() {
+ fireEvent(this, 'selected', { selected: this.selected });
+ }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ans-4175 begini kurang lebih cara saya nambahin event emitter (?) di komponennya

Copy link
Owner

Choose a reason for hiding this comment

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

okay, berarti nanti dibacanya di atasnya onselected sama seperti yang radio group button

Copy link
Contributor Author

Choose a reason for hiding this comment

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

iyak, betul

@imballinst imballinst marked this pull request as ready for review November 19, 2021 14:17
@imballinst imballinst changed the title [wip] feat(text randomizer): implement text randomizer and toggle between wheel feat(text randomizer): implement text randomizer and toggle between wheel Nov 19, 2021
@ans-4175
Copy link
Owner

image

Pas di randomize text, kalau ngerandom lagi dia lebih dahulu nampilin hasil dibanding randomnya beres. Ada logic yang kelewat?

@ans-4175
Copy link
Owner

image

Dilihat-lihat, apakah mungkin tab-textnya di tengah ya?

Copy link
Owner

@ans-4175 ans-4175 left a comment

Choose a reason for hiding this comment

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

Ada beberapa logic yang shown merchant detail ini, nampaknya callback react-nya ga pas cycle-nya

@@ -20,7 +20,8 @@
"build": "CI=false && react-scripts build",
"test": "react-scripts test",
"eject": "react-scripts eject",
"prettify": "prettier --write src"
"prettify": "prettier --write src",
"postinstall": "patch-package"
Copy link
Owner

Choose a reason for hiding this comment

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

hoo baru tahu begini, berarti ini auto jalan?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

iyak, jadi urutannya:

yarn --> node_modules terinstall (raw) --> patch-package (apply diff dari patches yang ada)

@@ -0,0 +1,113 @@
diff --git a/node_modules/wired-elements/lib/wired-lib.js b/node_modules/wired-elements/lib/wired-lib.js
Copy link
Owner

Choose a reason for hiding this comment

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

cara bikinnya ini gimana ji? BIkin file perubahannya, terus di diff sendiri dan hasil diffnya masukkin sini?

Copy link
Owner

Choose a reason for hiding this comment

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

naming filenya juga?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

o ini ada caranya, maap, habis ini saya tambahin di READMEnya

}
draw(svg, s) {
- rectangle(svg, 2, 2, s[0] - 4, s[1] - 4, this.seed);
+ if (this.hasBorder) {
Copy link
Owner

Choose a reason for hiding this comment

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

haha nice

patches/wired-elements-react+0.1.5.patch Show resolved Hide resolved
src/App.css Show resolved Hide resolved
src/App.js Outdated
setFetched(false);
setMustSpin(false);
refetch();
resetStates();
Copy link
Owner

Choose a reason for hiding this comment

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

yap nice

src/App.js Show resolved Hide resolved
<Wheel
mustStartSpinning={mustSpin}
mustStartSpinning={
randomizerMode === 'wheel' && mustStartRandomizing
Copy link
Owner

Choose a reason for hiding this comment

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

oh nice

src/App.js Outdated
<p className="txt-resto text-center">{pickedMerchant.name}</p>
{detailMerchant !== undefined &&
pickedMerchant !== undefined &&
isMerchantDetailShown && (
Copy link
Owner

Choose a reason for hiding this comment

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

coba cek logic isMerchantDetailShown ini keubah dimana ya? Jadi muncul duluan sebelum beres stop spin atau randomizer

<div>{optionsList[0].option}</div>

<div className="randomizer-text-helper">
Press "Randomize" button below to start.
Copy link
Owner

Choose a reason for hiding this comment

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

wkwk, anti pattern. Dia berharap ada tombol click di bawah di luar dari component ini

Copy link
Contributor Author

@imballinst imballinst Nov 20, 2021

Choose a reason for hiding this comment

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

wkwkwk, bener juga sih... apa kalau gitu mending dibuat gini?

  1. Tombol spin dan tombol reset jadi 1 komponen sendiri: SpinnerButtons
  2. Bikin komponen baru: WheelRandomizer -- yang ngeimport SpinnerButtons
  3. Import SpinnerButtons di TextBlinkRandomizer yang ada sekarang
  4. Delete spinner buttons yang ada di App.js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Saya jadinya pakai pattern children, kira-kira gimana? Saya senangnya pattern ini karena kita nggak perlu nambahin prop sih:

// App.js.
const spinnerButtons = mustStartRandomizing ? (
  <p>Waiting to {randomizeText}...</p>
) : (
  <div>
    <WiredButton elevation={2} onClick={handleSpinClick}>
      {randomizeText}
    </WiredButton>
    <WiredButton
      className="btn-reset"
      elevation={2}
      onClick={handleResetClick}
    >
      RE-LOAD
    </WiredButton>
  </div>
);

<TextBlinkRandomizer
  onFinishRandomizing={onFinishRandomizing}
  mustStartRandomizing={
    randomizerMode === 'text' && mustStartRandomizing
  }
  pickedMerchant={pickedMerchant}
  optionsList={optionsList}
>
  {spinnerButtons}
</TextBlinkRandomizer>

// TextBlinkRandomizer.js.
if (shownText === undefined) {
  return (
    <>
      <div className="randomizer-text-container">
        <div>{optionsList[0].option}</div>

        <div className="randomizer-text-helper">
          Press "Randomize" button below to start.
        </div>
      </div>

      {children}
    </>
  );
}

Copy link
Owner

Choose a reason for hiding this comment

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

oh nice ini, dia berarti selalu dibungkus sesuatu. sip, betul jadi ga ngerubah prop sebelumnya

@imballinst
Copy link
Contributor Author

image Pas di randomize text, kalau ngerandom lagi dia lebih dahulu nampilin hasil dibanding randomnya beres. Ada logic yang kelewat?

...wah, my bad, OK saya cek lagi

image Dilihat-lihat, apakah mungkin tab-textnya di tengah ya?

o iya bener, karena semuanya tengah, kayaknya lebih tepat di tengah. saya cobain

@ans-4175
Copy link
Owner

ans-4175 commented Nov 20, 2021 via email

Signed-off-by: Try Ajitiono <[email protected]>
Signed-off-by: Try Ajitiono <[email protected]>
@imballinst
Copy link
Contributor Author

oiya deng, dia pakai styled, hm. ini buildnya kenapa eror yak @ans-4175, boleh tolong dicekin?

Signed-off-by: Try Ajitiono <[email protected]>
Copy link
Owner

@ans-4175 ans-4175 left a comment

Choose a reason for hiding this comment

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

but checking on build error first. Karena patch-package
image
Seharusnya urutannya?
yarn, yarn postinstall, gitu kan?

README.md Outdated
To apply the patches under the [patches](./patches) folder, do this in the root of this project:

```shell
yarn patch-package
Copy link
Owner

Choose a reason for hiding this comment

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

yarn postinstall bukan harusnya?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

o bener, yarn postinstall aja biar ngereference ke scripts, siap

setDetailMerchant(undefined);
setMustStartRandomizing(true);
// Disable the tabs.
tabsRef.current.requestUpdate();
Copy link
Owner

Choose a reason for hiding this comment

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

sip

}
}, [merchants]);

const randomizeText = randomizerMode === 'wheel' ? 'Spin' : 'Randomize';
const isMerchantDetailShown =
Copy link
Owner

Choose a reason for hiding this comment

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

oh nice

@@ -0,0 +1,25 @@
import { Wheel } from 'react-custom-roulette';

export default function WheelRandomizer({
Copy link
Owner

Choose a reason for hiding this comment

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

nice pattern here

@ans-4175
Copy link
Owner

image

Ini logicnya kita cek lagi, aku `spin` dan `randomize`-nya belum sesuai show rendernya

@ans-4175
Copy link
Owner

image

Sudah bisa, aku perbaiki command install di vercel-nya, biar bisa patch package

@ans-4175
Copy link
Owner

Aku agak bingung di patch, ini dia masalah pertama yang kutemui, onselected ini ga trigger event deh, aku coba pasang debug console.log function, ga kepanggil
image
Aku belum terlalu ngerti bikin 'diff' itu

@imballinst
Copy link
Contributor Author

Seharusnya urutannya?
yarn, yarn postinstall, gitu kan?

iyak

image Ini logicnya kita cek lagi, aku `spin` dan `randomize`-nya belum sesuai show rendernya

...bener, wah maaf 🤦 kemarin bugnya adalah ketika beres spin, terjadi race condition sehingga detail merchantnya nggak muncul (karena onChangeTab entah kenapa kepanggil juga)... terus saya remove beberapa fireSelected, terlihat bisa, eh ternyata yang textnya ngaco

image Sudah bisa, aku perbaiki command install di vercel-nya, biar bisa patch package

siap, thanks!

@imballinst
Copy link
Contributor Author

imballinst commented Nov 21, 2021

@ans-4175 harusnya sih udah bener, ya... tapi buildnya masih gagal, kenapa ya?

2021-11-21.08-49-45.mov

saya pengen nambahin test, tapi ditolak sama jest, soalnya si wired-elements ini dia targetnya es2017 bukan es2015 (atau lebih rendah)... jadi hasilnya masih ada keyword export (yang nggak ada di JS biasa) 😅 https://github.com/rough-stuff/wired-elements/blob/master/tsconfig.json#L3

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.

Random Name Picker
2 participants