Bug 2032587 - Added UI for importing bookmarks#220
Bug 2032587 - Added UI for importing bookmarks#220fmasalha wants to merge 1 commit intomozilla-firefox:autolandfrom
Conversation
|
View this pull request in Lando to land it once approved. |
fmasalha
force-pushed
the
feature/import-bookmarks-ui
branch
from
e636f03 to
2ac3c42
Compare
segunfamisa
left a comment
segunfamisa
left a comment
There was a problem hiding this comment.
I think we're almost there, but there are a couple of changes I would like to recommend.
I also think we should move this over to phabricator, since it touches strings.xml files. Or is that no longer applicable?
| data object SelectFolderFailed : SnackbarAction() | ||
| } | ||
|
|
||
| internal data object RootMenuClicked : BookmarksAction |
There was a problem hiding this comment.
What's root menu in this case? I think "overflow menu" is probably more common so maybe "RootOverflowMenu...." is best here.
| val filePickerLauncher = rememberLauncherForActivityResult( | ||
| contract = ActivityResultContracts.GetContent(), | ||
| ) { _ -> } | ||
|
|
||
| LaunchedEffect(state.launchFilePicker) { | ||
| if (state.launchFilePicker) { | ||
| filePickerLauncher.launch("text/html") | ||
| store.dispatch(FilePickerDismissed) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
We can make this into a FilePicker.kt file and move it out of BookmarksScreen.kt. This file is already way too big.
| tint = MaterialTheme.colorScheme.onSurface, | ||
| ) | ||
| } | ||
| } else if (LocalContext.current.settings().importBookmarksFeatureFlagEnabled) { |
There was a problem hiding this comment.
Let's put something like showBookmarksImport in the state. We should not be accessing settings or data sources directly in the UI.
|
|
||
| if (state is EmptyListState.NotAuthenticated) { | ||
| Spacer(modifier = Modifier.height(FirefoxTheme.layout.space.static300)) | ||
| if (LocalContext.current.settings().importBookmarksFeatureFlagEnabled) { |
There was a problem hiding this comment.
Same here. Let's put this in the state instead.
| <!-- Label for button to sort bookmarks in reverse alphabetical order in the bookmark sorting overflow menu. --> | ||
| <string name="bookmark_sort_menu_z_to_a">Sort by Z to A</string> | ||
| <!-- Label for button to import bookmarks from file. --> | ||
| <string name="bookmark_import_menu_button">Import from file</string> |
There was a problem hiding this comment.
This PR should go on phabricator because it touches strings.
There was a problem hiding this comment.
Ahhh I forgot lol. I wonder how much longer we are gonna forced to use phab for patches that touch strings.
There was a problem hiding this comment.
LOL. I hope not for too long!
2 participants
No description provided.