refactor for toml no null #108
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| [tool.poetry] | ||
| name = "st-pages" | ||
| version = "1.0.2" | ||
| license = "MIT" | ||
| description = "An experimental version of Streamlit Multi-Page Apps" | ||
| authors = ["Zachary Blackwood <[email protected]>"] | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -76,7 +76,7 @@ def _get_nav_from_toml( | |
|
|
||
| for page in pages: | ||
| if page.is_section: | ||
| if hasattr(page, "icon") and page.icon != "": | ||
| current_section = f"{translate_icon(page.icon)} {page.name}" | ||
| else: | ||
| current_section = cast(str, page.name) | ||
|
|
@@ -89,7 +89,7 @@ def _get_nav_from_toml( | |
| st.Page( | ||
| page.path, | ||
| title=page.name, | ||
| icon=translate_icon(page.icon if hasattr(page, "icon") else None), | ||
| url_path=page.url_path, | ||
| ) | ||
| ) | ||
|
|
@@ -104,7 +104,7 @@ def _get_nav_from_toml( | |
| st.Page( | ||
| page.path, | ||
| title=page.name, | ||
| icon=translate_icon(page.icon if hasattr(page, "icon") else None), | ||
| url_path=page.url_path, | ||
| ) | ||
| ) | ||
|
|
@@ -132,12 +132,12 @@ def _add_page_title(page: StreamlitPage): | |
| Adds the icon and page name to the page as an st.title. | ||
| """ | ||
| page_title = page.title | ||
| page_icon = translate_icon(page.icon if hasattr(page, "icon") else None) | ||
|
|
||
| if page_icon and "/" in page_icon: | ||
| page_icon = None | ||
|
|
||
| st.title(f"{page_icon} {page_title}" if hasattr(page, "icon") else page_title) | ||
|
|
||
|
|
||
| add_page_title = gather_metrics("st_pages.add_page_title", _add_page_title) | ||
|
|
@@ -199,7 +199,9 @@ def _get_pages_from_config(path: str = ".streamlit/pages.toml") -> list[Page] | | |
| for page in raw_pages: | ||
| if page.get("is_section"): | ||
| page["path"] = "" | ||
| pages.append( | ||
| Section(page["name"], page["icon"] if hasattr(page, "icon") else None) | ||
| ) # type: ignore | ||
| else: | ||
| pages.append(Page(**page)) # type: ignore | ||
|
There was a problem hiding this comment. What do you think about handling "icon" here the same way you've handled it in the "if section" block above? That seems like it would nicely handle a lot of the other checks, and could simply default to null. There was a problem hiding this comment. sure, I will work on it tom and get back ! There was a problem hiding this comment. I have made the changes to set icon to None when initializing here but I think the string null check has to be done separately when setting the page title and section names. Let me know if that looks good! |
||
|
|
||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be the only required change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah..got it..cool..I will make the changes in some time!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done now ..thanks for the help!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are two fixes left:
here - It needs to be
if page.icon:instead as icon defaults to '' (empty string) herehere
- It needs to be page.get("icon") - so that the section icons get rendered
Let me know your thoughts - I can make the changes later today
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The icon should not default to
'', but toNone-- are you sure it's defaulting to''?Yeah,
page.get("icon")should work well thereThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, cause I see the leading space issue in the section name alone in this case,though am not sure why it defaults to ‘’ instead of None