Changing embed badge dropdown to a modal. #1066
Conversation
|
Created a new layout for mobile ( viewport width <=425px) for making the modal responsive. modal3.mp4 |
sd1p
changed the title
|
That seems odd looking to me on mobile. At that width probably it should just be a full-screen modal I think. |
Julian
added
the
waiting for author
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
|
Added overflow-x-scroll on card content to maintain viewport width within full screen size modal4.mp4 |
Julian
removed
the
waiting for author
for more information, see https://pre-commit.ci
|
Apologies for the force pushes, my intention was to squash several commits, but it didn't go as planned. I have added an external CSS file to manage the background color of the code block depending on the theme. |
|
All fine I don't mind either way. Is this ready for review then? |
Yeah. Please go ahead. |
|
It still is narrower than before, and therefore looks awkward to me. It looks like to make the modal wider at this point there is no Can you also explain why you're explicitly defining colors (in CSS)? What color is missing from where without that, doing so seems very suspicious as we're using a theme for the code block, so those colors would be wrong with any theme change? |
| const badgeEmbed = activeFormat.generateEmbed(activeBadge); | ||
|
|
||
| const compareBadges = (a: Badge, b: Badge) => { |
There was a problem hiding this comment.
If you're going to define this (which I previously considered but decided it wasn't really any better than just relying on the name alone for now) I think it belongs in data/Badge, especially if/when we have a badge class rather than just an interface. But feel free also to leave it as it was before, I don't mind either way personally.
There was a problem hiding this comment.
I have removed the comparator function and reverted the changes. Thanks.
I attempted to increase the size of the code-block, by adding padding to the code block and also added padding to it parent container, which required using explicit background color. I will try to implement it in a better way without explicitly declaring colors in CSS. I will also increase the width of the modal so it doesn't look squarish. Thanks for the feedback. |
|
I have made the modal more wider and removed hardcoded CSS colors.
|
for more information, see https://pre-commit.ci
|
In the previous commit the code block content overflow is not looking good. I tried to use I have checked the wrap code lines for all 5 available formats, everything looks good to me.
|
|
This looks good I think! Going to merge after a minor spacing fix. Really appreciate all your work! |
* main: Minor jiggering to the current dialect resource. build(deps): bump JsonSchema.Net build(deps): bump serde in /implementations/rust-jsonschema And now ensure we generate the same number of results as tests. Hypothesis-generate boolean schemas only for dialects supporting them.
Julian
removed
the
waiting for author
@Julian I was experimenting around making the embed badge UI responsive using modal instead of a dropdown, resolving issue #1043. The new layout looks great when the viewport width is greater than 425px.
I was thinking, for mobile displays, can we have a different layout?
modal1.mp4
📚 Documentation preview 📚: https://bowtie-json-schema--1066.org.readthedocs.build/en/1066/