Fix for details button misbehaving on mobile #1114
Conversation
* Move render methods to the last (together) in the order list * Remove unused component import * React lifecycle to come first before our own methods * General styling and eslint changes * Cleaner renderContent method using switch/case (fixes an eslint error as well!) * Cleaner renderPill method with proper spacing + removing uncessary else
* Use newer Fragment syntax * Remove unnecessary else statement * Use backtick strings for concatenating strings * Remove unnecessary space * Remove unused imports and variable declarations * Bunch render methods together as last in the order list
This _mostly_ fixes one of the issues being tracked on plausible#972, titled "Details button issue on Firefox specifically"
MaybeThisIsRu
changed the title
There was a problem hiding this comment.
Works great! The button no longer overlaps and I really like that you've added it some extra spacing for the button in mobile. Fat-fingered users like myself with appreciate it :)
All the quality-of-life improvements are also much appreciated ❤️
Looking through the code, 2 main things stand out to me.
- I know people have differing opinions on TailwindCSS or utility-first CSS in general. On this project we've went the utility-based CSS route. I think we should completely commit to it because mixing multiple different styles is way worse than just picking one. The goal is to do everything we can using default Tailwind classes so ideally the
app.cssfile would be empty. I remember adding a.stats-itemclass because I couldn't do a specific height orcalc(50% - 6px)widths in Tailwind. So some things need to stay in the css file currently but everything that can be added directly to the elements should be added directly to keep a consistent style. I'd like to see how much stuff we can move out ofapp.cssand in the context of this PR, how much we can keep from being added to it.
Let me know what you think.
=== OUT OF SCOPE FOR THIS PR ===
I think we could get rid of the whole .stats-item class by using Tailwind's grid primitives instead of flex. It has sligthly worse browser support but I don't think we need to worry about IE in our dashboard.
- I also know it feels bad to duplicate the Tailwind classes over and over. This is something I've been very bad at doing but the idea in Tailwind is to extract something like a React component instead of a CSS class. (like
assets/js/dashboard/stats/more-link.js).
The codebase is really at a point where further duplication is pointless and some refactoring is needed. To explain my approach, I like to let duplication get pretty bad before tackling it. Once you have many duplicates of the same thing, it's much clearer to see how one should extract it into a reusable thing. Inspired by Sandi Metz :)
|
🎉 That's great to hear! Happy to write them as classes instead. It really helped me to name them so I could understand the structure, but I'm absolutely happy to make those changes now. I think we should be able to move most or all from the classes I introduced to Tailwind style declaration. :) As for the CSS grid property, I'm generally not too familiar with the vanilla syntax (and therefore Tailwind's), so I'll probably refrain from using that here. P.s. Solid link-drop there, perhaps I can take some of that into my work today. 😀 |
|
👍🏼 thanks for the precommit docs :) |
|
That should take care of the issue now, @ukutaht. :) |
|
Great, thanks @hirusi ! |
|
this is now live and the details buttons work all fine on my mobile firefox! thanks @hirusi! |
Changes
This is a "first pass" for the details button issue being tracked in #972 titled "Details button issue on Firefox specifically". I'd love some testing and feedback on this. I'm certain I may have missed something! 😅
While it includes a bunch of changes related to development documentation and (optionally) usage (detailed below), I'd love feedback on MaybeThisIsRu@4cec202, which is the one and only commit to ultimately build on and tackle the mobile responsiveness issue.
While I have tried to add some comments at some places, they were not a focus so far. If something is unclear, please let me know, and I'll work on adding some. Context is easy to lose. :)
For development related changes...
make installandmake server. They are documented in the CONTRIBUTING.md file. Since I am not familiar with the Phoenix framework, this saves me a ton of time whenever I boot up the project.pre-commit, which was PR'ed by @maco some six months ago. 🧡pre-commitrequires Python so that has also been added to theasdfconfig file.A further commit is pending for updating the Changelog.
Tests
Changelog
Documentation