You are viewing a single comment's thread from:
RE: 👨🏼💻 #Proposal-86: Change Log - PrimaryNavigation.jsx (and .scss)
Wow, you've invested a lot of time in the documentation. Thank you.
Some things are difficult to understand with just the code screenshots. So I can't comment much on that.
However, I would like to address three points:
- The function
toggleNavigationVisibility(navigationId, isVisible)obviously takes the new visible state of the element as the second parameter (in the function you add the classvisibleifisVisible == true). The same happens withfalse.
As far as I can see now, the second parameter could be unnecessary if you check in the function whether the classvisibleis contained inclassList. Of course, this only works if you really want to toggle. snapWidthis set to 760 by default. Should this be a fixed setting? Wouldn't it be better to use the media query breakpoints here? Or have I misunderstood the purpose of the variable?- The mathematical calculations are very extensive and may go beyond the scope of the component code. I don't know in which file you have stored the calculations (Or is it really all in PrimaryNavigation.jsx?). Perhaps it would be worth considering moving these calculations to the
app/utilsfolder.
Thank you so much for taking the time to read through. It did take a long time to document everything (both this post and the previous one) but in explaining the changes, it helped me to validate that they were correct and worthwhile. In a couple of cases, the changes were improved or unnecessary so the process helped to improve the solution too.
I've pushed my changes to GitHub now, so you'll be able to see the code in its entirety which will hopefully make sense.
That's a good point. It's not really a toggle since it doesn't take the existing value and change it to the opposite value. I'll rename the function to "setNavigationVisibility" so that this is clear. I've also updated this post to reflect this change.
You've correctly understood the purpose of this variable. The 760px is the same width as the "Medium" mixin value set in the _layout.css file, which is the snapping point that I use in the CSS.
The calculations are all within the PrimaryNavigation.jsx file as they're only relevant for evaluating when the navigation gets pinned to the top or bottom of the screen. The conditions for which CSS class is added (pin-top, pin-top-padded or pin-bottom) is the only thing that these control.
I don't think they'd be used in any other context so it makes sense to me to keep them where they are.
Thank you so much again for reading through and feeding back. I really appreciate it.
Thanks for your explanations.
I took a quick look and saw your locales. Did you do the translations with a tool?
Sorry for taking so long to reply. I used ChatGPT for the translations. I thought that this would work better than something like Google Translate because I could provide it with additional context. There were some existing items that were similar in the language files already which allowed me to validate if the translations seemed appropriate. I then visited each page and translated the translations back into English to see if they made sense too 🙂
I found "The first rule of Steemit", etc...
https://steemitwallet.com/change_password
I thought that the steemwallet code was separate from the condenser code so I'm a bit surprised by this.
I think I'll do the same... or just leave it in English. I don't know yet.
For Spanish I have been able to activate users to help with the translation, but apart from that not so many (better: none) have responded to my call...