Differential D3288
[web] [fix] remove border-radius in input type="submit" Authored by • benschac on Feb 24 2022, 12:51 PM. Tags None Referenced Files
Details I've got this error in my VSCode for a while. Not sure why -webkit prefix was used here, I'm assuming it's just really old css. Border radius is a widely supported css property. https://developer.mozilla.org/en-US/docs/Web/CSS/border-radius#browser_compatibilit) Update: We no-longer use these css declarations in the code, so I'm removing them. N/A
Diff Detail
Event TimelineComment Actions It was probably from some CSS unset I copied from way back. I'm guessing it's -webkit-specific because maybe Webkit was the only engine that applied border-radius by default to input[type='submit'] Comment Actions
Can we check where it's used and then make sure things continue to look expected on Safari? Comment Actions The only place <input type='submit' is used is ConfirmLeaveThreadModal and that component is never used in the codebase. I'm going to delete this line entirely. Comment Actions Confirming: After searching the code base the only place <input type="submit" is used is in <ConfirmLeaveThreadModal` which after grepping is not used in the code base. Comment Actions Just a question – in case we use an input[type='submit'] in the future (seems likely) – do we currently have a CSS reset in place that does the equivalent of these lines that are being removed? Comment Actions We don't have a CSS reset. I think having one would be a great idea. Comment Actions I was about to accept based on the claim that we don't have any input[type='submit'], but then figured I should double-check. Good thing I double-checked. I found input[type='submit'] in ConcurrentModificationModal, NewThreadModal, and ThreadSettingsModal (along with ConfirmLeaveThreadModal). @benschac, I think this is the second time in the last couple weeks you've missed something significant while git greping. We need to be able to rely on the claims you make in diffs... right now, I find myself having to double-check everything. Please try and spend some time analyzing what went wrong here. Comment Actions
I took a couple of minutes to try and figure out where my searching went wrong.  FWIR -- I probably grepped for the wrong thing along the lines of git grep "<input type="submit". Which wouldn't get me every instance of an input with the type submit. I should have grepped for something like: git grep "type=submit" I would have seen the same issue as you did. Going forward I'm going to grep a bit more intentionally and think more about how to search on the codebase. Looking back on it, it makes sense that I would miss something like this because of how I searched on it. Comment Actions Makes sense, glad to hear you figured out what went wrong and know how to improve. To unblock this diff, can you make sure that it's not causing any regressions in any of the above listed modals? Comment Actions Yep, I added (and linked) another diff as a parent to this diff updating all of the modal buttons to use the Button component and not an input field with a submit type. Comment Actions Going to re-request review now that all of the input elements using type="submit" (in the modals you mentioned above) have been updated in the parent diff. |