Feature/fds navigation mobile version

[Joonas-M-S]

Elikkäs pyrin olemaan rikkomatta mitään. Nyt tuo navigaatio menee navigaationapin taakse, kun ruudun koko on tarpeeksi pieni. Lisäsin myös tuommoisen mobileOrder propertyn, jonka avulla navigaation itemejä voi järjestellä haluamaansa järjestykseen mobiilinäkymässä. Tämä sen takia, että näyttäisi Fintrafficin projekteissa olevan sellainen tapa, että hakukenttä on sijoitettu navigaation oikeaan nurkkaan desktopilla, mutta mobiililla se on listan ensimmäisenä.

[Joonas-M-S]

Tuosta primary-tyyppisessä navigaatiopalkista puuttunee jokin margin/padding jostain. Storybookista näkee, että oikeanpuoleisin nav-item (Settings) on kiinni navigaatiopalkin reunassa. Vertailukohtana voi käyttää tätä: https://fintraffic-design.github.io/coreui-components/?path=/docs/navigation--docs

Totta! Hyvin hoksattu. Pitääpä käydä lisäämässä.

En ole ennen tuota Storybookkia käyttänytkään missään muussa projektissa, niin pitä vähän siihen tutustua kans tässä.

[rooperl]

Tuli vielä mieleen, että meidän käytössä tuo navigaatiopalkki on leveydeltään pienempi kuin tuo 768px, vaikka kyseessä on desktop-selaimessa käytettävä sovellus. Eli jos ei halua tuota kollapsointia niin onko oikea tapa antaa mobile-width-propertylle jokin iso arvo?

[Joonas-M-S]

Tuli vielä mieleen, että meidän käytössä tuo navigaatiopalkki on leveydeltään pienempi kuin tuo 768px, vaikka kyseessä on desktop-selaimessa käytettävä sovellus. Eli jos ei halua tuota kollapsointia niin onko oikea tapa antaa mobile-width-propertylle jokin iso arvo?

Juu eli, jos haluaa, että kollapsointi tapahtuu vaikkapa 1000px kohdalla, niin siitten näin:

<fds-navigation mobile-width=1000>

Tosiaan, myöhemmin javascriptillä tuon propertyn muuttaminen ei vaikuta mitään.

[ninopenttinen]

@Joonas-M-S laita uutta katselmointi pyyntöä kun oot saanu sovitut muutokset tehtyä

[Joonas-M-S]

Jään lomille, niin palaan tän ääreen kolmen viikon kuluttua :palm_tree:

[rooperl]

Mergesin mainin tähän branchiin ja poistin tokenVar-funktion käytön tässä: https://github.com/fintraffic-design/coreui-components/pull/40/commits/e4d441e47b7fea28588c579121253ac589cefa3f

Tämä liittyy coreui-css-repon muutoksiin, joiden myötä suurin osa tokenVar-kutsuista on korvattu noilla suorilla property-importeilla: https://github.com/fintraffic-design/coreui-css/commit/78becee4865f573c2dfc46cb1fb26921a300c696

[solita-sabinaf]

Hei! One question here, am I assuming correctly that this change can also work as a general drop-down menu functionality, not necessarily only for a narrow mobile screen width?

[ninopenttinen]

Sure - only screen width is considered when deciding between regular navbar and dropdown navbar. No mobile only features here as far as I can see.

This does make me think that using the "mobile"-naming is probably not the correct decision. We should probably use something more generic as property name, like verticalMenuThreshold or something like that.

[solita-sabinaf]

Thanks, that's great to hear!

[Haprog]

Hey, I'm new in the project but will be working on this actively for now. I'd just want to know what is the state of this PR so we can get this moving?

Seems like Roope already approved this but Nino's previous request for changes is still active/blocking.

If this is ready for merge, we can just dismiss Nino's review (or he can add approval) so we can get this merged.

@ninopenttinen are we ready to merge here or do you still want some changes here first?

[ninopenttinen]

@Haprog

Hey, I'm new in the project but will be working on this actively for now. I'd just want to know what is the state of this PR so we can get this moving?

Seems like Roope already approved this but Nino's previous request for changes is still active/blocking.

If this is ready for merge, we can just dismiss Nino's review (or he can add approval) so we can get this merged.

@ninopenttinen are we ready to merge here or do you still want some changes here first?

Personally I would like to take the use case that Sabina mentioned into consideration on the component API, by renaming the mobile related properties into something more generic. Would be preferable to do this now, since changing the API afterwards will cause projects that use it to break.

Also I want to note that I don't really have any authority over this repo, so this is just my suggestion, not a demand, you may dismiss my review if you disagree with it.

[solita-sabinaf]

By the way, about the use case I mentioned, it's actually quite a different feature that I don't see it intersecting with this PR anymore. I was meaning having drop downs per menu item like this:

This had been implemented for our project's needs and haven't been published in PR

[ninopenttinen]

Thanks for clarification, I still do think though, that using "mobile" in the properties is a bit misleading, as it is not a mobile only feature, but rather a responsiveness thingy. However I'm fine with either.

[solita-sabinaf]

Yup, I agree, that "mobile"-looking layout can still be useful for non-mobile scenarios as well

[ninopenttinen]

If we want to this to be a "mobile only" feature, then I think rather than using a pixel threshold for changing the navbar, I would just add a boolean property that decides if we should render this component in "mobile" or "desktop" mode.

[ninopenttinen]

Although if we were to go with that (separate mobile/desktop modes) I would then rather have entirely different components for those to keep the code cleaner.

[Haprog]

Thanks for the quick replies. I'll need to familiarize myself with this component code and API to have a more informed opinion on the changes. Sounds like naming could be improved.

Generally for component responsiveness and toggling between display modes based on available space I'd prefer container queries (if CSS is enough) or JS (e.g. ResizeObserver) if needed, preferring to make it automatic without need for manually toggling modes by user (app developer). Breakpoint may be configurable if necessary but it would be better if mode can be automatically deduced based on container and content size.

Also I want to note that I don't really have any authority over this repo, so this is just my suggestion, not a demand, you may dismiss my review if you disagree with it.

I don't assume any authority either (at least so far). Just going to try my best to make things better. This should be a productive collaboration between interested/related parties. 🙂

[Joonas-M-S]

Hi, sorry for not getting back to this earlier. So there are two changes that need to be made

• mobile -> verticalMenuThreshold
• change media query to container one I'll try to make those changes today

[Haprog]

The filenames and structure changed a bit in main since #59 was merged. I can help merge the changes into this branch.

[Haprog]

@ninopenttinen if this looks ok to you now, let's squash and merge this.