Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Component padding and spacing updates #4861

Merged
merged 34 commits into from Jun 16, 2022

Conversation

imjuangarcia
Copy link

@imjuangarcia imjuangarcia commented Jun 13, 2022

📚 Context

This PR starts to adjust some of our components (namely st.selectbox, st.multiselect, st.text_input, st.number_input, st.text_area, st.time_input, and st.date_input), to their ideal counterparts in this Figma file.

The adjustments are mostly related to paddings and margins, so they adapt to Tailwind's default spacing scale, but it also introduces adjustments on some other component states, such as the disabled variant for st_multiselect.

Only added screenshots for this disabled state change because the rest of the changes are smaller and harder to compare, but let me know if you want me to add those as well!

  • What kind of change does this PR introduce?

    • Bugfix
    • Feature
    • Refactoring
    • Other, please describe: Visual adjustments

🧠 Description of Changes

st.selectbox:

  • Updated paddingBottom and paddingTop on VirtualDropdown
  • Updated ControlContainer, IconsContainer, ValueContainer, and Popover overrides

st.multiselect:

  • Added a StyleUISelect styled component, to adjust the background color for tags in disabled states for st_multiselect items
  • Changes on UISelect overrides
  • Added disabled tag styles on theme/utils.ts

st.text_input:

  • Updated marginBottom on StyledWidgetLabel
  • Updated borderWidth, padding and lineHeight on TextInput
  • Updated fontSize on StyledWidgetInstructions
  • Updated disabled text in theme/utils.ts

st.number_input:

  • [Already updated on st.text_input]: Updated marginBottom on StyledWidgetLabel.
  • Updated padding, borderWidth and lineHeight on NumberInput
  • Updated CONTROLS_WIDTH and height for controls in /NumberInput/styled-components.ts

st.text_area:

  • [Already updated on st.text_input]: Updated marginBottom on StyledWidgetLabel.
  • Updated padding and lineHeight on TextArea
    • Created a <StyledTextAreaContainer> component, and added a rule to the newly created styled-components.ts file to override the borderWidth property

st.time_input:

  • Updated selectOverrides on TimeInput mostly to adjust padding, margin and borderWidth

st.date_input:

  • Updated overrides on Input nested component, mostly to adjust padding, and borderWidth

    • This is a breaking API change
    • This is a visible (user-facing) change

Revised:

Screen Shot 2022-06-13 at 8 35 48 AM

Screen Shot 2022-06-13 at 8 35 39 AM

Current:

Screen Shot 2022-06-13 at 8 38 09 AM

Screen Shot 2022-06-13 at 8 38 20 AM

🧪 Testing Done

  • Screenshots included
  • Added/Updated unit tests
  • Added/Updated e2e tests

🌐 References

Does this depend on other work, documents, or tickets?


Contribution License Agreement

By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

@imjuangarcia imjuangarcia requested a review from a team June 13, 2022 11:39
@imjuangarcia imjuangarcia marked this pull request as draft June 15, 2022 15:20
@imjuangarcia imjuangarcia marked this pull request as ready for review June 15, 2022 15:26
@imjuangarcia
Copy link
Author

@willhuang1997 Thanks for approving! I don't have the privileges to merge this PR, do you need anything else from me before merging it? Thanks!

@LukasMasuch
Copy link
Collaborator

I did a quick check as well. Do you know why the expander is reverted back to the old version in the snapshots (referring to this update: #4801)?

https://github.com/imjuangarcia/streamlit/blob/component-updates-06012022/frontend/cypress/snapshots/linux/2x/st_expander.spec.js/expanders-in-main-dark.snap.png

@imjuangarcia
Copy link
Author

@LukasMasuch Thanks for taking a closer look, and sorry about the st.expander snapshots. This PR changed the visuals of so many widgets, and I had to regenerate so many of them that I probably accidentally overrode them. They should be up to date now!

@LukasMasuch
Copy link
Collaborator

@imjuangarcia thanks for updating this! Btw. if you remove outdated snapshots and let the CI pipeline run, you can also download the updated snapshots directly from CircleCI. I also have a small script which I use to bulk download multiple screenshots at once. This is sometimes more convenient compared to other methods for updating the e2e snapshots :) :

from urllib.request import Request, urlopen
import json
from tqdm import tqdm
import os

BASE_PATH = "circle-ci-artifacts"
CIRCLE_CI_TOKEN = "<CIRCLE_CI_TOKEN>"
FILTER = "st_legacy_empty_dataframes"
CIRCLE_CI_URL = (
    "https://circleci.com/api/v1.1/project/github/streamlit/streamlit/44028/artifacts"
)

req = Request(CIRCLE_CI_URL)
req.add_header("Circle-Token", CIRCLE_CI_TOKEN)
req.add_header("Accept", "application/json")
artifacts = json.loads(urlopen(req).read().decode("utf-8"))

for artifact in tqdm(artifacts):
    path = os.path.join(BASE_PATH, artifact["path"])
    if FILTER not in path:
        continue
    req = Request(artifact["url"])
    req.add_header("Circle-Token", CIRCLE_CI_TOKEN)
    file = urlopen(req)
    os.makedirs(os.path.dirname(path), exist_ok=True)
    with open(path, "wb") as output:
        output.write(file.read())

@LukasMasuch
Copy link
Collaborator

I think this file was added by mistake and needs to be removed: https://github.com/streamlit/streamlit/blob/668b8514eff8c08db937e74cd4aba60bd7c31bbf/examples/run_on_save.pybak

@imjuangarcia
Copy link
Author

@LukasMasuch Thanks for the script! It's definitively helpful. Just deleted the .pybak file as well, not sure why that's getting created when I run the example scripts...

@LukasMasuch LukasMasuch merged commit 5d73266 into streamlit:develop Jun 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants