Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
.snowpack
.vscode/
Copy link
Contributor

@nobrayner nobrayner Feb 5, 2021

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

.DS_Store
build
node_modules

Expand Down
4 changes: 0 additions & 4 deletions .vscode/settings.json

This file was deleted.

2 changes: 1 addition & 1 deletion src/routes/raids/ViewRaid.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ const userStatSortNames = Object.keys(userStatSorts)

const ViewRaid = () => {
const { raidId } = useParams<{ raidId: string }>()
const documentData = useDocument<ViewRaidData>('raid-stats', raidId)
const documentData = useDocument<RaidStats>('raid-stats', raidId)

const [currentSort, setCurrentSort] = useState(userStatSortNames[0])

Expand Down
284 changes: 244 additions & 40 deletions src/routes/raids/index.tsx
Original file line number Diff line number Diff line change
@@ -1,54 +1,258 @@
import React from 'react'
/** @jsx jsx */
import * as React from 'react'
import { css, jsx } from '@emotion/react'

import { Link, useRouteMatch } from 'react-router-dom'
import LoadingSpinner from '#components/loadingSpinner'
import useCollection from '#utils/useCollection'
import type { WithID } from '#utils/useCollection'
import styled from '@emotion/styled'
import useFilter from '#utils/useFilter'

const AllRaids = () => {
const collectionData = useCollection<ViewRaidData>('raid-stats')
enum Tabs {
active,
completed,
}

function filterByDungon(raids: RaidStats[], dungeon: string) {
return raids.filter((raid) => raid.dungeon.includes(dungeon))
}

function AllRaids() {
const { data, error, loading } = useCollection<RaidStats>('raid-stats')
const [tab, setTab] = React.useState<Tabs>(Tabs.active)

// @ts-expect-error issue with type merging and interfaces https://github.com/microsoft/TypeScript/issues/15300
const [results, setFilterText] = useFilter<typeof data[0]>(
data ?? [],
filterByDungon,
)

if (collectionData.state === 'success') {
const data = collectionData.data
const handleTabChange = (nextTab: Tabs): React.MouseEventHandler => () => {
setTab(nextTab)
}

const getTabProps = (nextTab: Tabs) => ({
active: tab == nextTab,
className: 'tab',
onClick: handleTabChange(nextTab),
})

if (loading) return <LoadingSpinner />
if (error)
return (
<>
<h1>Raids</h1>
<h2>Active</h2>
<ul>
{data
.filter((r) => r.status === 'active')
.map((s) => (
<li key={s.id}>
<Link to={`/raids/${s.id}`}>
{s.title} | {s.dungeon}
</Link>
</li>
))}
</ul>
<h2>Completed</h2>
<ul>
{data
.filter((r) => r.status === 'completed')
.map((s) => (
<li key={s.id}>
<Link to={`/raids/${s.id}`}>
{s.title} | {s.dungeon}
</Link>
</li>
))}
</ul>
</>
<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>
)
Comment on lines +44 to 48
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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? 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, support an empty/not found case aswell


return (
<div css={{ maxWidth: 1024, margin: '0 auto', display: 'flex' }}>
<div css={{ flex: '2 1 0' }}>
<$Title>Raids</$Title>
<$Tabs css={{ marginTop: 'var(--space-4)' }}>
<$Tab {...getTabProps(Tabs.active)}>Active</$Tab>
<$Tab {...getTabProps(Tabs.completed)}>Complete</$Tab>
</$Tabs>
{tab === Tabs.active && (
<RaidsSelection
raids={results.filter((raid) => raid.status === 'active')}
/>
)}
{tab === Tabs.completed && (
<RaidsSelection
raids={results.filter((raid) => raid.status === 'completed')}
/>
)}
</div>
<div
css={css`
flex: 1 1 0;
display: none;
@media (min-width: 768px) {
display: block;
}
`}
>
<FilterAside setFilterValue={setFilterText} />
</div>
</div>
)
}

const github = (repo: string) => `https://github.com/${repo}`

interface RaidsSelectionProps {
raids: WithID<RaidStats>[]
}

function RaidsSelection(props: RaidsSelectionProps) {
const { raids } = props

return (
<div css={{ marginTop: 'var(--space-4)' }}>
{raids.map((raid) => (
<RaidCard key={raid.id} raid={raid} />
))}
</div>
)
}

interface RaidCardProps {
raid: WithID<RaidStats>
}

function RaidCard(props: RaidCardProps) {
const { raid } = props
const { url } = useRouteMatch()
return (
<$RaidCard key={raid.id}>
<$Link to={`${url}/${raid.id}`} />
<div>
<h3>{raid.title}</h3>
<a href={github(raid.dungeon)} target="_blank" rel="noreferrer">
{raid.dungeon}
</a>
</div>
</$RaidCard>
)
}

interface FilterAsideProps {
setFilterValue(filterValue: string): void
}
function FilterAside(props: FilterAsideProps) {
const { setFilterValue } = props

const handleChange: React.ChangeEventHandler<HTMLInputElement> = (e) => {
setFilterValue(e.currentTarget.value)
}

return collectionData.state === 'loading' ? (
<LoadingSpinner />
) : (
// Theoretically this can/should never be hit... But a fun message nonetheless
<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>
return (
<$Aside>
<h3>Filters</h3>
<div css={{ marginTop: 'var(--space-4)' }}>
<label htmlFor="dungeon fiter">
Dungeon
<$Input
placeholder="facebook/react"
name="dungeon filter"
onChange={handleChange}
/>
</label>
</div>
</$Aside>
)
}

export default AllRaids

const $Title = styled.h1`
padding-left: var(--space-4);
margin-top: var(--space-4);
`
interface $TabProps {
active: boolean
}
const $Tab = styled.button<$TabProps>`
all: unset;
flex-grow: 1;
flex-shrink: 1;
flex-basis: 0;
cursor: pointer;
text-align: center;
font-weight: ${(props) => (props.active ? 700 : 'inherit')};
& + & {
margin-left: var(--space-2);
}
@media (min-width: 768px) {
text-align: left;
}
`
const $Tabs = styled.div`
display: flex;
max-width: 425px;
& > .tab + .tab {
margin-left: var(--space-2);
}
@media (min-width: 768px) {
margin-left: var(--space-4);
}
`
const $Link = styled(Link)`
position: absolute;
top: 0;
left: 0;
right: 0;
bottom: 0;
&:active {
background: var(--gray-100);
}
`
const $RaidCard = styled.div`
position: relative;
box-sizing: border-box;
& + & {
margin-top: var(--space-3);
}
width: 100%;
max-width: 425px;
cursor: pointer;
background: var(--white);
border: 1px solid var(--gray-500);
padding: var(--space-4);
padding-bottom: var(--space-5);
box-shadow: var(--elevation-1);
transition: box-shadow 300ms ease-in-out;
& > div {
position: relative;
pointer-events: none;
z-index: 1;
h3 {
font-size: var(--h4);
line-height: 2;
}
a {
margin-top: var(--space-5);
color: var(--gray-400);
text-decoration: none;
padding: 2px;
pointer-events: all;
&:hover {
border-bottom: 1px blue solid;
color: blue;
}
}
}
&:hover {
box-shadow: var(--elevation-2);
}
@media (min-width: 768px) {
margin-left: var(--space-4);
}
`

const $Input = styled.input`
all: unset;
line-height: 2;
border: 1px solid var(--gray-500);
padding-left: var(--space-2);
width: 100%;
background: var(--white);
&::placeholder {
color: var(--gray-200);
}
`

const $Aside = styled.aside`
padding: var(--space-4);
label {
margin-top: var(--space-4);
}
label input {
margin-top: var(--space-2);
display: block;
max-width: 330px;
}
`
21 changes: 16 additions & 5 deletions src/styles/globalStyles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ const globalStyles = css`
--white: hsl(0, 0%, 100%);
--gray-500: hsl(0, 0%, 25%);
--gray-400: hsl(0, 0%, 40%);
--gray-300: hsl(0, 0%, 66%);
--gray-200: hsl(0, 0%, 80%);
--gray-100: hsl(0, 0%, 97%);

Expand Down Expand Up @@ -37,15 +38,15 @@ const globalStyles = css`
--h4: calc(var(--h5) / var(--text-scale-ratio)); // 25
--h5: calc(1em / var(--text-scale-ratio)); // 20
--small-text: calc(1em * var(--text-scale-ratio)); // 12.8
@media screen and (max-width: 1024px) {
@media screen and (max-width: 1023px) {
--text-scale-ratio: 0.85;
}
@media screen and (max-width: 768px) {
@media screen and (max-width: 767px) {
Comment on lines +41 to +44
Copy link
Contributor Author

Choose a reason for hiding this comment

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

when using max-width, -1px is required

Copy link
Member

Choose a reason for hiding this comment

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

for what?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@datner datner Feb 5, 2021

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 😅 😆

Copy link
Contributor Author

@datner datner Feb 5, 2021

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@datner datner Feb 7, 2021

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

--text-scale-ratio: 0.9;
}

/* Spacing (fibonacci) */
--space-unit: 1em;
--space-unit: 1rem;
--space-1: calc(0.25 * var(--space-unit));
--space-2: calc(0.5 * var(--space-unit));
--space-3: calc(0.75 * var(--space-unit));
Expand All @@ -54,18 +55,28 @@ const globalStyles = css`
--space-6: calc(3.25 * var(--space-unit));
--space-7: calc(5.25 * var(--space-unit));
@media screen and (max-width: 1024px) {
--space-unit: 0.8em;
--space-unit: 0.8rem;
}

/* Shadows */
--elevation-1: 0 1px 3px rgba(0, 0, 0, 0.12), 0 1px 2px rgba(0, 0, 0, 0.24);
--elevation-2: 0 3px 6px rgba(0, 0, 0, 0.16), 0 3px 6px rgba(0, 0, 0, 0.23);
--elevation-3: 0 10px 20px rgba(0, 0, 0, 0.19),
0 6px 6px rgba(0, 0, 0, 0.23);
--elevation-4: 0 14px 28px rgba(0, 0, 0, 0.25),
0 10px 10px rgba(0, 0, 0, 0.22);
--elevation-5: 0 19px 38px rgba(0, 0, 0, 0.3),
0 15px 12px rgba(0, 0, 0, 0.22);
}

html {
font-size: 100%;
background: var(--background);
}

body {
font-family: var(--body-font);
font-weight: 400;
background: var(--background);
color: var(--text-color);
line-height: 1.4;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import firebase from 'firebase/app'
import 'firebase/firestore'

const firebaseConfig = {
export const firebaseConfig = {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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!

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Suggested change
export const firebaseConfig = {
const firebaseConfig = {

apiKey: 'AIzaSyCAgs6SNew9kKKFgQh7NLkqHK1n9Akq-GM',
authDomain: 'raid-stats-c1d5a.firebaseapp.com',
databaseURL: 'https://raid-stats-c1d5a-default-rtdb.firebaseio.com',
Expand Down
Loading