refactor(AllRaids): Add new AllRaids page #78

datner · 2021-02-04T01:02:53Z

theres a lot going on, so take your time

this is to simplify working with firebase

it's still a work in progress, there is no aside and it's unrefactored yet

package.json

datner · 2021-02-04T01:11:50Z

src/styles/globalStyles.ts

+    @media screen and (max-width: 1023px) {
      --text-scale-ratio: 0.85;
    }
-    @media screen and (max-width: 768px) {
+    @media screen and (max-width: 767px) {


when using max-width, -1px is required

Yeah, it should be the other way around.
max-width for tablet size should be 1024 (or 64r/em), min-width would be +1px

min width = "as long as the width is higher, these apply" or, px > min-width
max width = "as long as the width is lower than or equal to this number, these apply, or, px =< max-height

therefore, we need to remove an extra-pixel. But don't take my word for it, use chrome devtools and play around with the tablet sizes

I'd agree with Cody here - a difference of one pixel really won't make a difference... So please use a round number 😬

what, this is not a matter of opinion lmao, it's the standard viewport of devices, it either works or doesn't, it didn't so I fixed it 😅 😆

either that, or switch to min-width, like I do in my own components btw

Well aware of what max and min width are, and tablet sizes. If I want to change styles for tablet size and down it would be max-width: 1024px because that is the viewport width of the tablet. If the max width is set to 1023 the styles won't be applied

so this is the desired bahavior?

.box { color: red; } @media (max-width: 1024px) { .box { color: blue; } }

so this is the desired result?

phone: blue

tablets: blue

tablets + 1px: red

large tablets: red

laptop: red

PC: red

I'd think that the result would be:

phone: blue

tablets: red

tablets + 1px: red

large tablets: red

laptop: red

PC: red

This whole conversation is a bit pointless now, IMO.

They are just arbitrary numbers... Can we stop bikeshedding now? Thanks 😅 💯

If you look at just Apple tablets, then there isn't even a clear consensus... https://yesviz.com/tablets.php So can we please just pick 1024 to appease my round (rem) number needs?

src/styles/globalStyles.ts

JacobMGEvans · 2021-02-04T18:42:07Z

src/utils/useCollection/index.ts

@@ -1,52 +1,48 @@
 import { useState, useEffect } from 'react'
-import firestore from '#utils/useFirestore'
+import firestore from '#utils/firestore'


Why was the naming convention changed?

it's not a hook, so the convention did not change, it was fixed

Lets make it one then
https://usehooks.com/useFirestoreQuery/

This wont block anything from being merged, but can be a TODO for sure

this hook is essentially what is implemented in useCollection logic-wise, I found that there is no use for a generic access to firestore............ for now

useCollection is specific, I would rather there is a useFirebase and is composed in more specific Custom Hooks like useCollection, useCharacters, useWhateverFirebaseData, or whatever. Likely the general Firebase logic was put into useCollection due to when it was written it being the only Firebase Query

There are two query style hooks so far - useDocument and useCollection - useCollection can probably be turned into a useFirestoreQuery hook keeping all the same logic.

useDocument could then be re-written to utilize useFirestoreQuery under-the-hood, but I think it will still be a useful abstraction for just grabbing one document by it's id - though I can understand not wanting it at all 😛

src/routes/raids/ViewRaid.tsx

JacobMGEvans · 2021-02-04T18:49:29Z

src/utils/useDocument/index.ts

@@ -1,5 +1,5 @@
 import { useState, useEffect } from 'react'
-import firestore from '#utils/useFirestore'
+import firestore from '#utils/firestore'


Same as above

Same as above.
https://usehooks.com/useFirestoreQuery/

types/viewRaidData.d.ts

src/styles/globalStyles.ts

nobrayner · 2021-02-04T20:03:18Z

src/utils/firestore/index.ts

@@ -1,7 +1,7 @@
 import firebase from 'firebase/app'
 import 'firebase/firestore'

-const firebaseConfig = {
+export const firebaseConfig = {


This shouldn't need to be exported... It's intentionally only exposing firestore - you shouldn't be using the config anywhere else.

artefacts from when I tried to work with reactfire, the config shouldn't be committed in the first place if you want to actually discuss this

In Firebase documentation the frontend config can be exposed, if you actually want to discuss this. https://firebase.google.com/docs/web/setup

Note: The Firebase config object contains unique, but non-secret identifiers for your Firebase project.
Visit Understand Firebase Projects to learn more about this config object.

I stand corrected!

Just remove the export, and this would be A-OK for me

Suggested change

export const firebaseConfig = {

const firebaseConfig = {

src/utils/useCollection/index.ts

also remove .DS_Store stuff it's mostly for personal ref stuff

note, it's hidden for mobile because theres no design

@JacobMGEvans

as per @JacobMGEvans suggestion

nobrayner · 2021-02-05T20:50:14Z

.gitignore

@@ -1,4 +1,6 @@
 .snowpack
+.vscode/


You may not use VS Code, but the majority does

This folder, and the settings file inside, help to provide prompts to VS Code so that we can get consistent behaviour across people using it.

This should NOT be removed

I actually changed to vscode to be inline with how you guys work, and some of my extensions there started editing files in .vscode, I can can exclude the specific file that was edited by my instance instead of the entire folder, on it

nobrayner · 2021-02-05T20:53:02Z

src/routes/raids/index.tsx

+      <p>
+        {`Strange... I could've sworn the Manticore was here a second ago! Seems
+        there aren't any Raids right now. Try again later!`}
+      </p>
    )


This is what I was referring to by the not found/no items and error state no longer being distinguished.

This is NOT a good error message - this is why it never was one, and was used when the state was 'not-found'. Please add some no items checks, and actually display the error here instead so we maintain + improve functionality.

not-found does not return an error though, it returns an empty array from my tests

I can add a !data.length check if that is what you mean

not-found does not return an error though

Correct. The way you have it currently though, is that when there is an error, you are showing what I had previously as the "data.length === 0" case. Which means the actual error is being suppressed.

If you could add !data.length checks, and also a short message about there being no data, that would be perfect; combined with rendering the error message in the error state.

I hope I explained better that time? 😅

yeah, support an empty/not found case aswell

nobrayner · 2021-02-05T21:01:15Z

src/styles/globalStyles.ts

+    @media screen and (max-width: 1023px) {
      --text-scale-ratio: 0.85;
    }
-    @media screen and (max-width: 768px) {
+    @media screen and (max-width: 767px) {


I'd agree with Cody here - a difference of one pixel really won't make a difference... So please use a round number 😬

src/styles/globalStyles.ts

nobrayner · 2021-02-05T21:03:07Z

types/viewRaidData.d.ts

@@ -7,7 +7,7 @@ interface UserStats {
  commits: number
 }

-interface ViewRaidData {
+interface RaidStats {


Should probably rename the file to raidStats.d.ts while we are at it 🤔

Yuval Datner added 11 commits January 30, 2021 23:17

feat(firebase): add reactfire

3f09a03

this is to simplify working with firebase

Merge branch 'main' of github.com:OpenSourceRaidGuild/website into main

edcf995

style(global): change to rem and add elevation

35feb9c

refactor(firestore): change firesore implementaion

c46d45f

feat(useFilter): add useFilter

72819ed

chore(App): remove unused imports

dbf6f52

refactor(Raid): change type to Raid

44a78e8

feat(AllRaid): add new AllRaids screen

f9b1688

it's still a work in progress, there is no aside and it's unrefactored yet

Merge branch 'main' of github.com:OpenSourceRaidGuild/website into main

3732e85

Merge remote-tracking branch 'origin/main' into datner/all-raids

2ba6116

chore(yarn.lock): remove unrelated registry

f16174f

datner commented Feb 4, 2021

View reviewed changes

package.json Outdated Show resolved Hide resolved

datner commented Feb 4, 2021

View reviewed changes

JacobMGEvans requested a review from nobrayner February 4, 2021 18:37

JacobMGEvans added Status: In Progress 🚧 Currently taking out the trash mobs or cleaning up the dungeon Type: Feature New Feature and/or functionality - significant code rewrite or additions labels Feb 4, 2021

JacobMGEvans linked an issue Feb 4, 2021 that may be closed by this pull request

All Raids Screen #17

Open

JacobMGEvans reviewed Feb 4, 2021

View reviewed changes

src/styles/globalStyles.ts Show resolved Hide resolved

JacobMGEvans reviewed Feb 4, 2021

View reviewed changes

src/routes/raids/ViewRaid.tsx Outdated Show resolved Hide resolved

JacobMGEvans reviewed Feb 4, 2021

View reviewed changes

types/viewRaidData.d.ts Outdated Show resolved Hide resolved

JacobMGEvans requested a review from codyarose February 4, 2021 18:50

nobrayner reviewed Feb 4, 2021

View reviewed changes

src/styles/globalStyles.ts Show resolved Hide resolved

nobrayner reviewed Feb 4, 2021

View reviewed changes

Yuval Datner added 4 commits February 5, 2021 11:32

chore(gitignore): remove .vscode directory from git

cfcf513

also remove .DS_Store stuff it's mostly for personal ref stuff

feat(AllRaids): add aside

66abfc4

note, it's hidden for mobile because theres no design

style(globalStyles): change em to rem

8ec534b

build(package.json): remove reactfire

f8b1011

datner requested review from JacobMGEvans and nobrayner February 5, 2021 09:59

datner marked this pull request as ready for review February 5, 2021 10:00

Yuval Datner added 2 commits February 5, 2021 14:41

Merge branch 'main' into datner/all-raids

5b7e4d7

refactor(*): rename Raid to RaidStats

594ab1f

as per @JacobMGEvans suggestion

nobrayner requested changes Feb 5, 2021

View reviewed changes

JacobMGEvans mentioned this pull request Feb 6, 2021

Create a Generic useFirebase and **compose** it in more specific Custom Hooks likeuseWhateverData #80

Open

JacobMGEvans added Status: Pending ⏳ Status: Stale labels Feb 9, 2021

JacobMGEvans requested a review from nobrayner February 24, 2021 14:09

JacobMGEvans added Status: Abandoned and removed Status: Stale labels Feb 24, 2021

JacobMGEvans closed this Feb 28, 2021

JacobMGEvans deleted the datner/all-raids branch October 26, 2021 20:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(AllRaids): Add new AllRaids page #78

refactor(AllRaids): Add new AllRaids page #78

datner commented Feb 4, 2021

datner Feb 4, 2021

JacobMGEvans Feb 4, 2021

codyarose Feb 5, 2021

datner Feb 5, 2021 •

edited

Loading

nobrayner Feb 5, 2021

datner Feb 5, 2021

datner Feb 5, 2021 •

edited

Loading

codyarose Feb 6, 2021

datner Feb 7, 2021 •

edited

Loading

nobrayner Feb 8, 2021

JacobMGEvans Feb 4, 2021

datner Feb 5, 2021 •

edited

Loading

JacobMGEvans Feb 5, 2021

JacobMGEvans Feb 5, 2021

datner Feb 5, 2021

JacobMGEvans Feb 6, 2021

nobrayner Feb 6, 2021

JacobMGEvans Feb 4, 2021

JacobMGEvans Feb 5, 2021

nobrayner Feb 4, 2021

datner Feb 5, 2021

JacobMGEvans Feb 5, 2021

datner Feb 5, 2021

nobrayner Feb 5, 2021

nobrayner Feb 5, 2021 •

edited

Loading

datner Feb 5, 2021

nobrayner Feb 5, 2021

datner Feb 5, 2021

datner Feb 5, 2021

nobrayner Feb 6, 2021

datner Feb 7, 2021

nobrayner Feb 5, 2021

nobrayner Feb 5, 2021

datner Feb 5, 2021

refactor(AllRaids): Add new AllRaids page #78

refactor(AllRaids): Add new AllRaids page #78

Conversation

datner commented Feb 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

datner Feb 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

datner Feb 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

datner Feb 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

datner Feb 5, 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

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

nobrayner Feb 5, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

datner Feb 5, 2021 •

edited

Loading

datner Feb 5, 2021 •

edited

Loading

datner Feb 7, 2021 •

edited

Loading

datner Feb 5, 2021 •

edited

Loading

nobrayner Feb 5, 2021 •

edited

Loading