style: enhancements for responsive design #930
Conversation
…929-responsive-design-improvment
…der in between groups
pyphilia
left a comment
pyphilia
left a comment
There was a problem hiding this comment.
Thanks for your PR, I think it can be optimized in various, please take a look at my comments! 🐰
|
spaenleh
left a comment
spaenleh
left a comment
There was a problem hiding this comment.
Nice work ! I have left a lot of comments on how to improve and refine this work.
I think you should:
- fix the
.only - fix any small issues that do not require a significant refactor.
We will then merge the PR and plan a refactor to refine the components and architecture.
This will allow us to move on from this PR (which has been stuck for a while, sorry) and build upon it in a clean way, how does that sound ?
| const { id } = SAMPLE_ITEMS.items[1]; | ||
| cy.setUpApi(SAMPLE_ITEMS); | ||
| visitItem({ id }); | ||
|
|
There was a problem hiding this comment.
These lines are repeated in each test, put them in the beforeEach.
| const displayTags = cy.get( | ||
| `#${buildCoEditorSettingsRadioButtonId(option.value)}`, | ||
| ); | ||
| displayTags.should('be.visible'); |
There was a problem hiding this comment.
Do not assign to a variable. It is prefered to simply chain.
| const displayTags = cy.get( | |
| `#${buildCoEditorSettingsRadioButtonId(option.value)}`, | |
| ); | |
| displayTags.should('be.visible'); | |
| cy.get( | |
| `#${buildCoEditorSettingsRadioButtonId(option.value)}`, | |
| ).should('be.visible'); |
| }, | ||
| ); | ||
| }); | ||
| it.only('Recycle item', () => { |
| visitItem({ id }); | ||
| cy.get(`#${MOBILE_MORE_ACTIONS_BUTTON_ID}`).click(); | ||
| cy.get(`.${ITEM_MENU_FLAG_BUTTON_CLASS}`).click(); | ||
| cy.get(`#${buildFlagListItemId(type)}`).should('exist'); |
There was a problem hiding this comment.
There is a difference between should exist and should be visible:
exist: only checks for existence in the DOM, if it is disabled, or hidden by CSS this will passbe.visible: asserts that it is visible by the user, if it is hidden by CSS this will fail.
In general you want to assert for visibility more than existence.
| visitItem({ id }); | ||
| cy.get(`#${MOBILE_MORE_ACTIONS_BUTTON_ID}`).click(); | ||
| cy.get(`.${ITEM_SETTINGS_BUTTON_CLASS}`).click(); | ||
| cy.get(`#${ITEM_SETTINGS_CONTAINER_ID}`).should('exist'); |
|
|
||
| export const MobileItemHeaderActions = (): JSX.Element => { | ||
| const { t: translateBuilder } = useBuilderTranslation(); | ||
| const itemId = useShortenURLParams(ITEM_ID_PARAMS); |
There was a problem hiding this comment.
This is deprecated, use useParams()[ITEM_ID_PARAMS] instead.
| <Typography px={3} pb={1} variant="h5"> | ||
| {item.name} | ||
| </Typography> | ||
| <Divider sx={{ borderColor: theme.palette.grey[400] }} /> |
There was a problem hiding this comment.
I do not think you need a divider here. if you really need one you can simply change the opacity:
| <Divider sx={{ borderColor: theme.palette.grey[400] }} /> | |
| <Divider sx={{ opacity: 0.8 }} /> |
for example.
| <Box | ||
| display="flex" | ||
| flexDirection="column" | ||
| sx={{ maxHeight: '100%', overflow: 'auto', marginTop: 1 }} | ||
| > | ||
| {renderItemActions()} | ||
| </Box> |
There was a problem hiding this comment.
For this part you can use the Stack component as it has a divider prop that lets you specify the component that should be used to separate the children.
| <Box | |
| display="flex" | |
| flexDirection="column" | |
| sx={{ maxHeight: '100%', overflow: 'auto', marginTop: 1 }} | |
| > | |
| {renderItemActions()} | |
| </Box> | |
| <Stack | |
| flexDirection="column" | |
| divider={<Divider variant="middle" flexItem />} | |
| > |
spaenleh
force-pushed
the
main
branch
2 times, most recently
from
0791683 to
b513c2f
Compare
closes graasp/client#671