julkascript / cardflow

The open-source Trading Card Game market
MIT License
10 stars 4 forks source link

beta alert div coded #134

Closed Alperadoss closed 22 hours ago

Alperadoss commented 4 days ago

PR for issue : https://github.com/julkascript/cardflow/issues/128

I translated to Bulgarian language with Google Translate, and am not sure if it is correct translation. Can you please check?

Screenshot 2024-07-01 at 17 41 23

julkascript commented 3 days ago

Thanks a ton for your contribution! I wrote some comments you can address. @RyotaMitaraiWeb may also have some remarks.

Overall you’re in the right direction!

RyotaMitaraiWeb commented 3 days ago

edit: worth mentioning that I didn't test any of this locally, since this PR seems to be committing directly to develop and I am not sure how / if it is possible to pull them locally before merging

Alperadoss commented 3 days ago

Thanks for directing @julkascript and @RyotaMitaraiWeb . I got one unresolved issue. When i apply tailwindCSS in Alert it doesn't apply in anyway. I tried to wrap with a div and apply tailwind to that but it didn't work too. I also had a look at other places in repo where "className=" is used but didn't see sth to solve this issue. Can you please help with this one?

Dear @julkascript if you also advise as @RyotaMitaraiWeb I can remove useState and remove hiding alert div.

julkascript commented 3 days ago

@julkascript has to weigh in about this, but I feel like being able to hide the alert seems pointless, since navigating back to the page will restore it anyways.

I agree. Handling state for the Alert is unnecessary. You can remove the state and let the Alert be there all the time.

@Alperadoss can you please give a specific code example of what you tried with tailwind, so we can help?

Alperadoss commented 2 days ago

I removed useState. Tailwind CSS code example is below.

` <Alert id="closable-alert" severity="info" className="text-black text-lg w-5/6 mx-auto my-4 bg-[#E5F6FD] flex font-light border rounded-lg items-center"

{t('main.betaWarning')}

`

RyotaMitaraiWeb commented 2 days ago

Managed to pull the changes locally

What seems to trouble @Alperadoss based on what I see on my end is the fact that the font class names do not apply:

alert

If we care enough about matching the font exactly as on the wireframe (I personally think MUI's default is fine, but I will let others judge), you need to wrap up the Trans component in another tag (e.g. a <p>) and apply the classes there

edit: here's an example of how it'd look:

<p className="text-black">
  <Trans>
    blah blah blah
  </Trans>
</p>
julkascript commented 2 days ago

Please stick to the default font (MUI’s default) and default font colors. The wireframe id just an example, sorry if it brings confusion.

RyotaMitaraiWeb commented 2 days ago

In that case, only remove the classes that visibly don't do anything (as to not litter the component with useless class names) and I will approve

Alperadoss commented 2 days ago

I removed the classes which doesn't make change regarding @julkascript 's message. Screenshot 2024-07-03 at 18 00 33

julkascript commented 1 day ago

This change was planned for v1.4. However, since it's done, it looks good and it's a small change - I'm happy to actually include it in v1.3, once my latest comment gets addressed.