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

Antti/frontend_fix #15

Merged
merged 24 commits into from
Mar 17, 2024
Merged

Antti/frontend_fix #15

merged 24 commits into from
Mar 17, 2024

Conversation

anttiasmala
Copy link
Collaborator

@anttiasmala anttiasmala commented Mar 3, 2024

Tein nopean "korjauksen" fronttiin. Sivusta tuli melko käyttökelvoton, jos oli liian pitkä nimi ja/tai lahja. Tämä ei ole hieno toteutus, mutta nyt sivua pitäisi olla mahdollista käyttää 😄

Edit: Vielä sen voi rikkoa, jos laittaa todella paljon merkkejä, mutta pitäisi olla silti varmempi 😄

Copy link

vercel bot commented Mar 3, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
lahjalista ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 14, 2024 7:51pm

@anttiasmala anttiasmala requested a review from samuliasmala March 3, 2024 15:07
Copy link
Owner

@samuliasmala samuliasmala left a comment

Choose a reason for hiding this comment

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

Mielestäni pakko-rivitys kuten alin lahja kuvassa on parempi kuin scrolli. Sen saa tehtyä overflow-wrap: anywhere; css-komennolla (ei taida löytyä Tailwindistä). Kun sen lisää li elementille, niin samalla whitespace-nowrap overflow-auto luokat pitää ottaa pois.

Tässä ohje, miten toimia jos css-komento ei löydy Tailwindistä.

Lisäsin myös hieman paddingiä, reunoille, nyt mobiilissa menee aivan ruudun reunaan kiinni.

image

pages/_app.tsx Outdated
Comment on lines 6 to 16
return (
<>
<Head>
<meta
name="viewport"
content="width=device-width, initial-scale=1.0, interactive-widget=resizes-visual"
/>
</Head>
<Component {...pageProps} />
</>
);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Onko tämmöinen suotavaa? Tämä korjasi ongelman, jonka kuvasin Discordissa. En löytänyt / keksinyt tähän mitään muuta ratkaisua kuin tämän

Copy link
Owner

Choose a reason for hiding this comment

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

Ilmeisesti joku näistä korjasin ongelman:

content="width=device-width, initial-scale=1.0, interactive-widget=resizes-visual"

Tiedätkö mikä oli ratkaiseva, vai tarvittiinko kaikki?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ilmeisesti joku näistä korjasin ongelman:

content="width=device-width, initial-scale=1.0, interactive-widget=resizes-visual"

Tiedätkö mikä oli ratkaiseva, vai tarvittiinko kaikki?

width=device-width: tämä lienee turha. En ainakaan saanut toistettua mitään eroja sen kanssa tai ilman

initial-scale=1.0: tämä on melkeinpä pakollinen. Laitan kuvan 😄

interactive-widget=resizes-visual: resizes-visual pitäisi olla resizes-content. 😄 Mutta tämä näyttäisi olevan se joka korjasi ongelman. Yritin kokeilla kaikki keinot läpi, jolla saisin toistettua jonkinongelman tuota käyttäessä. En onnistunut mitään bugeja löytämään 😄

Eli TLDR, tämä on toimiva rivi:

content="initial-scale=1.0, interactive-widget=resizes-content"

example

Copy link
Owner

Choose a reason for hiding this comment

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

Kiitos! Täytyy sanoa että en tiedä miksi interactive-widget=resizes-content korjaa ongelman, mutta hyvä kun se tekee sen. Ja tiedoksi, että sain toistettua ongelman myös omalla puhelimellani.

pages/index.tsx Outdated
@@ -141,18 +141,15 @@ export default function Home() {
<Button type="submit">Lisää</Button>
</form>
</div>
<div className="mt-3 w-[20rem]">
<div className="mt-3 w-full sm:w-[20rem]">
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Olisiko parempi antaa myös tietokoneilla width-arvon olla full?

<body className="bg-gray-300">
<body className="bg-white">
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Olisiko tämän hyvä olla erivärinen kuin "tavallinen" taustaväri?

Copy link
Owner

Choose a reason for hiding this comment

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

Annetaan olla näin kun teit nyt. Ulla tekee pian designiin visuaalisia parannuksia, niin muokataan tätä sitten tarvittaessa.

@anttiasmala
Copy link
Collaborator Author

Nyt pitäisi olla kaikki korjattuna. Tosiaan frontti ei lähde sitten yhtään, sitä on tullut niin vähän katsottua, mutta pikku hiljaa hyvä tulee 😅

overflow-wrap: anywhere; Tämä oli hyvä lisä, kiitos!

@anttiasmala anttiasmala requested a review from samuliasmala March 5, 2024 09:51
<body className="bg-gray-300">
<body className="bg-white">
Copy link
Owner

Choose a reason for hiding this comment

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

Annetaan olla näin kun teit nyt. Ulla tekee pian designiin visuaalisia parannuksia, niin muokataan tätä sitten tarvittaessa.

pages/index.tsx Outdated
Comment on lines 177 to 186
onMouseOver={(e) =>
e.currentTarget.parentElement?.setAttribute(
'class',
'underline',
)
}
onMouseOut={(e) =>
e.currentTarget.parentElement?.removeAttribute('class')
}
className="m-3 ml-0 p-0 w-20 h-8 hover:text-yellow-400"
Copy link
Owner

Choose a reason for hiding this comment

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

Näitä onMouseOver ja onMouseOut tarvitsee todella harvoin. Itse en muista koskaan käyttäneeni. Kun eikö tämänkin voi korvata hover:underline luokalla buttoniin?

Suggested change
onMouseOver={(e) =>
e.currentTarget.parentElement?.setAttribute(
'class',
'underline',
)
}
onMouseOut={(e) =>
e.currentTarget.parentElement?.removeAttribute('class')
}
className="m-3 ml-0 p-0 w-20 h-8 hover:text-yellow-400"
className="m-3 ml-0 p-0 w-20 h-8 hover:text-yellow-400 hover:underline"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Näitä onMouseOver ja onMouseOut tarvitsee todella harvoin. Itse en muista koskaan käyttäneeni. Kun eikö tämänkin voi korvata hover:underline luokalla buttoniin?

Tämän tarkoitus on ala- tai yliviivata lahja riippuen onko kursori Muokkaa- vai Poista-napin päällä.

Lyhykäisyydessään tämä asettaa lahjan div-wrapperille line-through- tai underline-arvon

className="m-3 ml-0 p-0 w-20 h-8 hover:text-yellow-400 hover:underline"

Tällä tavalla yritin itsekin ratkaista, mutta tämä tapa alleviivaa pelkästään tuon nappulan tekstin

Olisiko tässä kohtaa järkevin antaa tuon onMouseOver & onMouseOut -tavan olla vai kannattaako se poistaa?

Copy link
Owner

@samuliasmala samuliasmala Mar 14, 2024

Choose a reason for hiding this comment

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

Oho totta, en katsonut tarpeeksi tarkasti. Vähän innostuin selvittämään tuota että saako tehtyä pelkällä CSS:llä vai onko tuo sun tapa ainoa. Luulen että sain toimivan CSS-version joka ei tosin ollut ihan simppeli. Pushasin siitä commitin, tsekkaapa toimiiko kuten pitää.

EDIT: .hover-target:has(~ .trigger-line-through:hover) siis tarkoittaa, että CSS-sääntöä käytetään jos elementillä jolla on hover-target luokka on sibling (rinnakkainen) elementti jolla on trigger-line-through luokka ja kyseinen elementti on hover-tilassa.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Vau! Toimii loistavasti! Kiitos vielä tuosta selvennyksestä, nyt tiedän miten tarkalleen ottaen toimii! :)

pages/index.tsx Outdated
@@ -141,48 +141,57 @@ export default function Home() {
<Button type="submit">Lisää</Button>
</form>
</div>
<div className="mt-3">
<div className="pl-8 pr-8 mt-3 w-full sm:w-[20rem]">
Copy link
Owner

Choose a reason for hiding this comment

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

Yleensä on hyvä käyttää valmiita kokoja, esim. tuon sun sm:w-[20rem] voi korvata sm:max-w-xs. Mutta nyt myös isoilla näytöillä 20 rem on maksimikoko -> muuttaisin niin että isoilla näytöillä sisältöä näkyy enemmän. Itse asiassa pienillä näytöillä kokoa ei mun mielestä tarvitse rajoittaa, vaan on hyvä käyttää koko näytön tila. Eli itse laittaisin koon seuraavasti:

Suggested change
<div className="pl-8 pr-8 mt-3 w-full sm:w-[20rem]">
<div className="pl-8 pr-8 mt-3 w-full max-w-xl">

pages/_app.tsx Outdated
Comment on lines 6 to 16
return (
<>
<Head>
<meta
name="viewport"
content="width=device-width, initial-scale=1.0, interactive-widget=resizes-visual"
/>
</Head>
<Component {...pageProps} />
</>
);
Copy link
Owner

Choose a reason for hiding this comment

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

Kiitos! Täytyy sanoa että en tiedä miksi interactive-widget=resizes-content korjaa ongelman, mutta hyvä kun se tekee sen. Ja tiedoksi, että sain toistettua ongelman myös omalla puhelimellani.

Copy link
Owner

@samuliasmala samuliasmala left a comment

Choose a reason for hiding this comment

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

Valmis mergettäväksi!

@anttiasmala anttiasmala merged commit 0512bb9 into main Mar 17, 2024
4 of 5 checks passed
@anttiasmala anttiasmala deleted the antti/frontend-fix branch March 17, 2024 14:55
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.

2 participants